Skip to content

Merge LargeFileManager into FileManager - #1671

Open
yuvipanda wants to merge 2 commits into
jupyter-server:mainfrom
yuvipanda:large-fix
Open

yuvipanda wants to merge 2 commits into
jupyter-server:mainfrom
yuvipanda:large-fix

Conversation

@yuvipanda

Copy link
Copy Markdown
Contributor

Per #899, this was out of an abundance of caution.

Fixes #899

Per jupyter-server#899,
this was out of an abundance of caution.

Fixes jupyter-server#899

@krassowski krassowski left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

The test failures appear related:

FAILED tests/services/contents/test_largefilemanager.py::test_bad_save[LargeFileManager-model1-HTTP 400: Bad Request (File type "notebook" is not supported for large file transfer)] - assert False
 +  where False = expected_http_error(<ExceptionInfo HTTPError() tblen=2>, 400, expected_message='HTTP 400: Bad Request (File type "notebook" is not supported for large file transfer)')
FAILED tests/services/contents/test_largefilemanager.py::test_bad_save[AsyncLargeFileManager-model1-HTTP 400: Bad Request (File type "notebook" is not supported for large file transfer)] - assert False
 +  where False = expected_http_error(<ExceptionInfo HTTPError() tblen=3>, 400, expected_message='HTTP 400: Bad Request (File type "notebook" is not supported for large file transfer)')

@@ -13,143 +13,10 @@
class LargeFileManager(FileContentsManager):
"""Handle large file upload."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding a future deprecation warning in the docstring and in the __init__ explaining that one can use FileContentsManager directly?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I think a deprecation notice makes sense -- basically just say this is already the new default and we'll remove this in v3.

@dlqqq

dlqqq commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

lol, I was "like why does this sound so familiar?", then I opened the original issue :)

Thanks for contributing this! Seems very sensible to me. I think some of the test failures are related though, which is strange since the code looks safe. I can help give this another look if folks are busy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

proposal: merge LargeFileManager into FileContentsManager

3 participants