Skip to content

proposal: merge LargeFileManager into FileContentsManager #899

Description

@dlqqq

Problem

I was tracing the ContentsManager implementation to find out how I can attach some logic to its copy/rename/move methods. According the dev docs, the default implementation of the ContentsManager abstract class is FileContentsManager.

After wasting a lot of time looking through serverapp.py to find where FileContentsManager was being initialized, I found that FileContentsManager is not the default implementation, but LargeFileManager is, initialized here as a default value for the contents_manager_class traitlet:

    # REMOVE in VERSION 2.0
    # Temporarily allow content managers to inherit from the 'notebook'
    # package. We will deprecate this in the next major release.
    contents_manager_class = TypeFromClasses(
        default_value=LargeFileManager,
        klasses=[
            "jupyter_server.services.contents.manager.ContentsManager",
            "notebook.services.contents.manager.ContentsManager",
        ],
        config=True,
        help=_i18n("The content manager class to use."),
    )

Digging through the source, it looks like LargeFileManager just extends the save method on FileContentsManager. That got me thinking: is there any reason to keep both LargeFileManager and FileContentsManager in our source tree? I simply don't see a use case where a developer would want to exclusively use FileContentsManager and ignore handling of large files. The extra level of inheritance makes the save() logic harder to trace, and just seems unnecessary. The 2.0 release is a good opportunity to clean up the codebase.

Proposed Solution

Merge the two classes together, and drop LargeFileManager. Same goes for AsyncLargeFileManager. More precisely, we can add a private method FileContentsManager#_save_large_file() and modify the existing FileContentsManager to have another block:

chunk = model.get("chunk", None)
if chunk is not None:
    self._save_large_file(...)
    return model

If we're against adding more logic to the 900-line FileContentsManager class, we can migrate file save logic to a helper or util function somewhere else. I'm just against inheritance being the favored approach for separating application logic here.

P.S.

One final note: can we drop "notebook.services.contents.manager.ContentsManager" from klasses in the traitlet definition?

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions