Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up handling of synchronous managers #1044

Merged
merged 10 commits into from
Nov 3, 2022

Conversation

blink1073
Copy link
Contributor

Make the async contents manager the default, and add deprecation warning when using synchronous contents or kernel managers.

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 73.08%. Comparing base (4b2ac37) to head (a4d0b81).
Report is 313 commits behind head on main.

Files with missing lines Patch % Lines
jupyter_server/pytest_plugin.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
- Coverage   73.10%   73.08%   -0.03%     
==========================================
  Files          64       64              
  Lines        8274     8269       -5     
  Branches     1643     1642       -1     
==========================================
- Hits         6049     6043       -6     
- Misses       1816     1820       +4     
+ Partials      409      406       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1435,38 +1433,13 @@ def template_file_path(self):
help="""If True, display controls to shut down the Jupyter server, such as menu items or buttons.""",
)

# REMOVE in VERSION 2.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Zsailer I believe removing this is what is causing the nbclassic downstream test to fail. Do we want to keep this in for another major release?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right.

Yes. We can wait another release. I don't see any major urgency to removing this now, we just don't want to keep this around forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, punted to 3.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that was not it. Now I'm confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so the actual issue occurs when we change the default value from LargeFileManager -> AsyncLargeFileManager, which was the main idea of this change. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nbclassic was assuming synchronous contents manager:

  tests/test_notebookapp.py::test_tree_handler
    /home/runner/test_venv/downstream_test/nbclassic/nbclassic/tree/handlers.py:53: RuntimeWarning: coroutine 'AsyncFileContentsManager.dir_exists' was never awaited
      if cm.dir_exists(path=path):
    Enable tracemalloc to get traceback where the object was allocated.
    See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
  
  tests/test_notebookapp.py::test_tree_handler
    /home/runner/test_venv/downstream_test/nbclassic/nbclassic/tree/handlers.py:54: RuntimeWarning: coroutine 'AsyncFileContentsManager.is_hidden' was never awaited

We need to update it regardless.

@blink1073
Copy link
Contributor Author

All green!

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

Thanks, @blink1073! Great stuff here!

@blink1073 blink1073 enabled auto-merge (squash) November 3, 2022 22:21
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.

3 participants