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
12 changes: 6 additions & 6 deletions jupyter_server/pytest_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
from jupyter_server.auth import Authorizer
from jupyter_server.extension import serverextension
from jupyter_server.serverapp import JUPYTER_SERVICE_HANDLERS, ServerApp
from jupyter_server.services.contents.filemanager import FileContentsManager
from jupyter_server.services.contents.largefilemanager import LargeFileManager
from jupyter_server.services.contents.filemanager import AsyncFileContentsManager
from jupyter_server.services.contents.largefilemanager import AsyncLargeFileManager
from jupyter_server.utils import url_path_join

# List of dependencies needed for this plugin.
Expand Down Expand Up @@ -488,14 +488,14 @@ def jp_kernelspecs(jp_data_dir):

@pytest.fixture(params=[True, False])
def jp_contents_manager(request, tmp_path):
"""Returns a FileContentsManager instance based on the use_atomic_writing parameter value."""
return FileContentsManager(root_dir=str(tmp_path), use_atomic_writing=request.param)
"""Returns an AsyncFileContentsManager instance based on the use_atomic_writing parameter value."""
return AsyncFileContentsManager(root_dir=str(tmp_path), use_atomic_writing=request.param)


@pytest.fixture
def jp_large_contents_manager(tmp_path):
"""Returns a LargeFileManager instance."""
return LargeFileManager(root_dir=str(tmp_path))
"""Returns an AsyncLargeFileManager instance."""
return AsyncLargeFileManager(root_dir=str(tmp_path))


@pytest.fixture
Expand Down
53 changes: 20 additions & 33 deletions jupyter_server/serverapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import gettext
import hashlib
import hmac
import inspect
import ipaddress
import json
import logging
Expand Down Expand Up @@ -57,8 +56,8 @@
if not sys.platform.startswith("win"):
from tornado.netutil import bind_unix_socket

from jupyter_client import KernelManager
from jupyter_client.kernelspec import KernelSpecManager
from jupyter_client.manager import KernelManager
from jupyter_client.session import Session
from jupyter_core.application import JupyterApp, base_aliases, base_flags
from jupyter_core.paths import jupyter_runtime_dir
Expand Down Expand Up @@ -123,7 +122,7 @@
AsyncFileContentsManager,
FileContentsManager,
)
from jupyter_server.services.contents.largefilemanager import LargeFileManager
from jupyter_server.services.contents.largefilemanager import AsyncLargeFileManager
from jupyter_server.services.contents.manager import (
AsyncContentsManager,
ContentsManager,
Expand All @@ -133,7 +132,6 @@
MappingKernelManager,
)
from jupyter_server.services.sessions.sessionmanager import SessionManager
from jupyter_server.traittypes import TypeFromClasses
from jupyter_server.utils import (
check_pid,
fetch,
Expand Down Expand Up @@ -1164,7 +1162,7 @@ def _deprecated_password_config(self, change):
config=True,
help="""Disable cross-site-request-forgery protection

Jupyter notebook 4.3.1 introduces protection from cross-site request forgeries,
Jupyter server includes protection from cross-site request forgeries,
requiring API requests to either:

- originate from pages served by this server (validated with XSRF cookie and token), or
Expand Down Expand Up @@ -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.

# 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",
],
contents_manager_class = Type(
default_value=AsyncLargeFileManager,
klass=ContentsManager,
config=True,
help=_i18n("The content manager class to use."),
)

# Throws a deprecation warning to notebook based contents managers.
@observe("contents_manager_class")
def _observe_contents_manager_class(self, change):
new = change["new"]
# If 'new' is a class, get a string representing the import
# module path.
if inspect.isclass(new):
new = new.__module__

if new.startswith("notebook"):
self.log.warning(
"The specified 'contents_manager_class' class inherits a manager from the "
"'notebook' package. This is not guaranteed to work in future "
"releases of Jupyter Server. Instead, consider switching the "
"manager to inherit from the 'jupyter_server' managers. "
"Jupyter Server will temporarily allow 'notebook' managers "
"until its next major release (2.x)."
)

kernel_manager_class = Type(
klass=MappingKernelManager,
config=True,
Expand Down Expand Up @@ -1865,6 +1838,20 @@ def init_configurables(self):
# this determination, instantiate the GatewayClient config singleton.
self.gateway_config = GatewayClient.instance(parent=self)

if not issubclass(self.kernel_manager_class, AsyncMappingKernelManager):
warnings.warn(
"The synchronous MappingKernelManager class is deprecated and will not be supported in Jupyter Server 3.0",
DeprecationWarning,
stacklevel=2,
)

if not issubclass(self.contents_manager_class, AsyncContentsManager):
warnings.warn(
"The synchronous ContentsManager classes are deprecated and will not be supported in Jupyter Server 3.0",
DeprecationWarning,
stacklevel=2,
)

self.kernel_spec_manager = self.kernel_spec_manager_class(
parent=self,
)
Expand Down
2 changes: 1 addition & 1 deletion jupyter_server/services/contents/filemanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ async def save(self, model, path=""):
"""Save the file model and return the model with no content."""
path = path.strip("/")

self.run_pre_save_hook(model=model, path=path)
self.run_pre_save_hooks(model=model, path=path)

if "type" not in model:
raise web.HTTPError(400, "No file type provided")
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ filterwarnings = [
"error",
"ignore:Passing a schema to Validator.iter_errors:DeprecationWarning",
"ignore:run_pre_save_hook is deprecated:DeprecationWarning",

"always:unclosed <socket.socket:ResourceWarning",
"module:Jupyter is migrating its paths to use standard platformdirs:DeprecationWarning",
]

Expand Down
12 changes: 12 additions & 0 deletions tests/services/contents/test_api.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import json
import pathlib
import sys
import warnings
from base64 import decodebytes, encodebytes
from unicodedata import normalize

Expand All @@ -14,6 +15,17 @@
from ...utils import expected_http_error


@pytest.fixture(autouse=True)
def suppress_deprecation_warnings():
with warnings.catch_warnings():
warnings.filterwarnings(
"ignore",
message="The synchronous ContentsManager",
category=DeprecationWarning,
)
yield


def notebooks_only(dir_model):
return [nb for nb in dir_model["content"] if nb["type"] == "notebook"]

Expand Down
4 changes: 2 additions & 2 deletions tests/services/contents/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

from jupyter_server.services.contents.checkpoints import AsyncCheckpoints
from jupyter_server.services.contents.filecheckpoints import (
AsyncFileCheckpoints,
AsyncGenericFileCheckpoints,
GenericFileCheckpoints,
)
from jupyter_server.services.contents.manager import AsyncContentsManager


@pytest.fixture(params=[AsyncGenericFileCheckpoints, GenericFileCheckpoints])
@pytest.fixture(params=[AsyncGenericFileCheckpoints, AsyncFileCheckpoints])
def jp_server_config(request):
return {"FileContentsManager": {"checkpoints_class": request.param}}

Expand Down
12 changes: 12 additions & 0 deletions tests/services/kernels/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import json
import os
import time
import warnings

import jupyter_client
import pytest
Expand All @@ -16,6 +17,17 @@
TEST_TIMEOUT = 60


@pytest.fixture(autouse=True)
def suppress_deprecation_warnings():
with warnings.catch_warnings():
warnings.filterwarnings(
"ignore",
message="The synchronous MappingKernelManager",
category=DeprecationWarning,
)
yield


@pytest.fixture
def pending_kernel_is_ready(jp_serverapp):
async def _(kernel_id, ready=None):
Expand Down
12 changes: 12 additions & 0 deletions tests/services/kernels/test_cull.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import json
import os
import platform
import warnings

import jupyter_client
import pytest
Expand All @@ -12,6 +13,17 @@
CULL_INTERVAL = 1


@pytest.fixture(autouse=True)
def suppress_deprecation_warnings():
with warnings.catch_warnings():
warnings.filterwarnings(
"ignore",
message="The synchronous MappingKernelManager",
category=DeprecationWarning,
)
yield


@pytest.mark.parametrize(
"jp_server_config",
[
Expand Down
12 changes: 12 additions & 0 deletions tests/services/sessions/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import os
import shutil
import time
import warnings
from typing import Any

import jupyter_client
Expand All @@ -22,6 +23,17 @@
TEST_TIMEOUT = 60


@pytest.fixture(autouse=True)
def suppress_deprecation_warnings():
with warnings.catch_warnings():
warnings.filterwarnings(
"ignore",
message="The synchronous MappingKernelManager",
category=DeprecationWarning,
)
yield


def j(r):
return json.loads(r.body.decode())

Expand Down