Set JPY_SESSION_NAME to full notebook path. #1100

Carreau · 2022-11-28T18:03:48Z

This also add some typing here and there, and extend one of the console warning to log an exception when there is an error.

My main concern is that get_kernel_env need to become async.

codecov · 2022-11-28T18:09:50Z

Codecov Report

Base: 79.93% // Head: 79.99% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (6c7a897) compared to base (a52709c).
Patch coverage: 91.30% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1100      +/-   ##
==========================================
+ Coverage   79.93%   79.99%   +0.05%     
==========================================
  Files          68       68              
  Lines        8124     8143      +19     
  Branches     1601     1604       +3     
==========================================
+ Hits         6494     6514      +20     
+ Misses       1205     1202       -3     
- Partials      425      427       +2

Impacted Files	Coverage Δ
jupyter_server/services/sessions/handlers.py	`79.20% <40.00%> (-1.95%)`	⬇️
jupyter_server/services/kernels/kernelmanager.py	`81.87% <90.00%> (+0.70%)`	⬆️
jupyter_server/base/handlers.py	`78.82% <100.00%> (ø)`
jupyter_server/gateway/managers.py	`83.37% <100.00%> (ø)`
jupyter_server/services/contents/fileio.py	`90.10% <100.00%> (ø)`
jupyter_server/services/sessions/sessionmanager.py	`88.36% <100.00%> (+0.36%)`	⬆️
jupyter_server/utils.py	`85.46% <100.00%> (+0.17%)`	⬆️
jupyter_server/serverapp.py	`80.19% <0.00%> (+0.26%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kevin-bates

Thanks @Carreau. I don't see an issue with making get_kernel_env() async since its essentially an internal method (i.e., so custom (and synchronous) SessionManagers should still work calling their "internal" implementation). I suppose things could be dicey if there is a server extension that is calling that method on the active SessionManager, but, hey, "major release boundaries" have their benefits. 😉

jupyter_server/services/sessions/handlers.py

jupyter_server/services/kernels/kernelmanager.py

kevin-bates · 2022-11-30T01:45:18Z

jupyter_server/services/sessions/sessionmanager.py

        "Create a uuid for a new session"
        return str(uuid.uuid4())

    async def create_session(
        self, path=None, name=None, type=None, kernel_name=None, kernel_id=None
    ):
-        """Creates a session and returns its model"""
+        """Creates a session and returns its model
+


Should we add parameter descriptions for the others, or remove the description for name?

I added the description of what I was sure of, but yes in general many of those function should get actual documentation.

jupyter_server/services/sessions/sessionmanager.py

kevin-bates · 2022-11-30T01:45:35Z

jupyter_server/services/sessions/sessionmanager.py

+        if name is not None:
+            session_dir = await self.contents_manager.get_kernel_path(path=path)
+            cwd = self.kernel_manager.cwd_for_path(path)
+            path = os.path.join(cwd, session_dir, name)
+        assert isinstance(path, str)


Won't path be the same value as name when the older form of the body is used (i.e., the one that includes a 'notebook' stanza)? And, if so, will that produce the expected results?

Any idea how "old" the old API is? I.e., does lab or nbclassic use that form of payload?

I honestly don't know. It's honestly really unclear for me why this is still called path if it's often (always?) not a path.

I'm also quite confused about the on-disk Path and Api Path which is why I started to add type annotations.

blink1073 · 2022-11-30T13:30:04Z

Ah, I have to fix CI separately for changes in pytest-jupyter

blink1073 · 2022-11-30T13:34:00Z

jupyter-server/pytest-jupyter#33

blink1073 · 2022-11-30T13:53:32Z

https://github.com/jupyter-server/pytest-jupyter/releases/tag/v0.5.3

Carreau · 2022-11-30T13:53:49Z

Ah, I have to fix CI separately for changes in pytest-jupyter

Thanks, some of the original failures were due to changes I've made, so no problem.

jupyter_server/services/sessions/handlers.py

jupyter_server/services/sessions/sessionmanager.py

kevin-bates · 2022-11-30T20:36:59Z

jupyter_server/services/sessions/sessionmanager.py

+        if name is not None:
+            session_dir = await ensure_async(self.contents_manager.get_kernel_path(path=path))
+            cwd = self.kernel_manager.cwd_for_path(path)
+            path = os.path.join(cwd, session_dir, name)
+        assert isinstance(path, str)
        return {**os.environ, "JPY_SESSION_NAME": path}


I don't think session_dir is necessary here. If the notebook file is in a subdirectory of root_dir, JPY_SESSION_NAME will contain a redundant directory name...

Removing session_dir from the equation would allow the method to be synchronous again.

That is true, thanks.

jupyter_server/services/sessions/sessionmanager.py

Carreau · 2022-12-01T09:39:01Z

pre-commit (pyupgrade) seem to insist on having optional types be X|Y which is only python 3.10 plus AFAICT.

blink1073 · 2022-12-01T11:38:45Z

That sounds like a bug, we have it configured to --py38-plus.

blink1073 · 2022-12-01T12:32:40Z

Okay, build failures seem fixed.

Carreau · 2022-12-01T13:25:14Z

Okay, build failures seem fixed.

Do you want me to rebase / squash ?

blink1073 · 2022-12-01T15:37:26Z

No thanks, squash is the default merge type

jupyter_server/services/sessions/sessionmanager.py

blink1073 · 2022-12-01T19:55:36Z

Note: we're not merging any API-changing PRs until after the 2.0 final release on Monday.

blink1073 · 2022-12-07T14:07:26Z

Heads up, this PR picked up a few conflicts from the linting refactor in #1114

Carreau · 2022-12-22T09:59:32Z

The linting fails but this is unrelated to this PR, some class have signature that change between the sub and superclass, in particular

async def _async_start_kernel(self, kernel_id=None,... with kernel_name in **kwargs, while in superclass (in jupyter_client) it is the opposite with kernel_name as first parameter and kernel_id in kwargs...

Carreau · 2022-12-22T10:05:16Z

There is also am async method in a subclass that is not async in superclass, and comparison between list and strings that is always false.

This also add some typing here and there, and extend one of the console warning to log an exception when there is an error. My main concern is that get_kernel_env need to become async. Co-authored-by: Kevin Bates <[email protected]>

Carreau · 2023-01-11T09:40:18Z

Rebased on master, all the typing error seem unrelated. Is there anything else I can do to help getting that in ?

blink1073 · 2023-01-11T13:21:43Z

jupyter_server/services/kernels/kernelmanager.py

@@ -196,12 +199,16 @@ async def _remove_kernel_when_ready(self, kernel_id, kernel_awaitable):
        self._kernel_connections.pop(kernel_id, None)
        self._kernel_ports.pop(kernel_id, None)

-    async def _async_start_kernel(self, kernel_id=None, path=None, **kwargs):
+    # TODO DEC 2022: Revise the type-ignore once the signatures have been changed upstream


Still todo?

The PR have been merged, but we likely need it to be released, and we may want o wait a bit to keep compat with older versions, and once we pin to a newer jupyter_client, we can force this one to be positional only as well.

Okay, let's move ahead, thanks!

Carreau · 2023-01-12T12:54:48Z

thanks !

…

On Thu, Jan 12, 2023 at 13:36 Steven Silvester ***@***.***> wrote: Merged #1100 <#1100> into main. — Reply to this email directly, view it on GitHub <#1100 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACR5T3XMMXAZHCKVHSSFG3WR73E3ANCNFSM6AAAAAASNTP32Y> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

Following jupyter-server/jupyter_server#1100, this expose the value of JUPYTER_SESSION_NAME environment variable as `__file__`. In _most_ cases, this should reflect the notebook file that correspond to current kernel. It is not set when the env variable is not set. And it represent the name of file at the time the session was created, it will not reflect renaming of the file. We could support renaming, but that would require the frontend to send the name of the current file; this would be helpful when we have multiple frontends/documents connected to the same kernel, but this would be out of scope for this PR and a longer discussion to have.

kevin-bates reviewed Nov 30, 2022

View reviewed changes

Carreau force-pushed the notebook-name branch 3 times, most recently from 0b3e4d5 to 2c0ddb0 Compare November 30, 2022 13:20

Carreau force-pushed the notebook-name branch from 39a38cd to 766b878 Compare November 30, 2022 13:56

kevin-bates reviewed Nov 30, 2022

View reviewed changes

jupyter_server/services/sessions/handlers.py Outdated Show resolved Hide resolved

kevin-bates reviewed Nov 30, 2022

View reviewed changes

jupyter_server/services/sessions/sessionmanager.py Outdated Show resolved Hide resolved

kevin-bates reviewed Nov 30, 2022

View reviewed changes

jupyter_server/services/sessions/sessionmanager.py Outdated Show resolved Hide resolved

kevin-bates reviewed Nov 30, 2022

View reviewed changes

jupyter_server/services/sessions/sessionmanager.py Outdated Show resolved Hide resolved

kevin-bates reviewed Nov 30, 2022

View reviewed changes

kevin-bates reviewed Dec 1, 2022

View reviewed changes

jupyter_server/services/sessions/sessionmanager.py Outdated Show resolved Hide resolved

Carreau force-pushed the notebook-name branch from 97454c3 to 4aeabaa Compare December 1, 2022 09:17

blink1073 added the enhancement label Dec 1, 2022

kevin-bates reviewed Dec 1, 2022

View reviewed changes

jupyter_server/services/sessions/sessionmanager.py Outdated Show resolved Hide resolved

kevin-bates reviewed Dec 1, 2022

View reviewed changes

jupyter_server/services/sessions/sessionmanager.py Outdated Show resolved Hide resolved

Carreau force-pushed the notebook-name branch from 2fdd280 to cc050b6 Compare December 21, 2022 14:54

Carreau force-pushed the notebook-name branch from dd7c80b to bfdd1d1 Compare December 23, 2022 11:44

Carreau force-pushed the notebook-name branch 2 times, most recently from 6d9c208 to a66e8df Compare January 4, 2023 13:25

Carreau mentioned this pull request Jan 5, 2023

Decide and stick once and for all if functions are async or not ? #1165

Open

Carreau force-pushed the notebook-name branch from d307840 to 0751241 Compare January 5, 2023 10:04

Carreau and others added 4 commits January 11, 2023 10:28

Set JPY_SESSION_NAME to full notebook path.

57b4568

This also add some typing here and there, and extend one of the console warning to log an exception when there is an error. My main concern is that get_kernel_env need to become async. Co-authored-by: Kevin Bates <[email protected]>

fix linter/typing

33e5099

workaround type ignore

d6b6169

fix a few lints

6c7a897

Carreau force-pushed the notebook-name branch from 0751241 to 6c7a897 Compare January 11, 2023 09:33

blink1073 reviewed Jan 11, 2023

View reviewed changes

blink1073 merged commit 3d516b3 into jupyter-server:main Jan 12, 2023

Carreau mentioned this pull request Jan 24, 2023

Expose session start file in __file__. ipython/ipykernel#1078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set JPY_SESSION_NAME to full notebook path. #1100

Set JPY_SESSION_NAME to full notebook path. #1100

Carreau commented Nov 28, 2022

codecov bot commented Nov 28, 2022 •

edited

Loading

kevin-bates left a comment

kevin-bates Nov 30, 2022

Carreau Nov 30, 2022

kevin-bates Nov 30, 2022

Carreau Nov 30, 2022

blink1073 commented Nov 30, 2022

blink1073 commented Nov 30, 2022

blink1073 commented Nov 30, 2022

Carreau commented Nov 30, 2022

kevin-bates Nov 30, 2022 •

edited

Loading

Carreau Dec 1, 2022

Carreau commented Dec 1, 2022

blink1073 commented Dec 1, 2022

blink1073 commented Dec 1, 2022

Carreau commented Dec 1, 2022

blink1073 commented Dec 1, 2022

blink1073 commented Dec 1, 2022

blink1073 commented Dec 7, 2022

Carreau commented Dec 22, 2022

Carreau commented Dec 22, 2022

Carreau commented Jan 11, 2023

blink1073 Jan 11, 2023

Carreau Jan 11, 2023

blink1073 Jan 12, 2023

Carreau commented Jan 12, 2023 via email

Set JPY_SESSION_NAME to full notebook path. #1100

Set JPY_SESSION_NAME to full notebook path. #1100

Conversation

Carreau commented Nov 28, 2022

codecov bot commented Nov 28, 2022 • edited Loading

Codecov Report

kevin-bates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blink1073 commented Nov 30, 2022

blink1073 commented Nov 30, 2022

blink1073 commented Nov 30, 2022

Carreau commented Nov 30, 2022

kevin-bates Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carreau commented Dec 1, 2022

blink1073 commented Dec 1, 2022

blink1073 commented Dec 1, 2022

Carreau commented Dec 1, 2022

blink1073 commented Dec 1, 2022

blink1073 commented Dec 1, 2022

blink1073 commented Dec 7, 2022

Carreau commented Dec 22, 2022

Carreau commented Dec 22, 2022

Carreau commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carreau commented Jan 12, 2023 via email

codecov bot commented Nov 28, 2022 •

edited

Loading

kevin-bates Nov 30, 2022 •

edited

Loading