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

Inject session identifier into environment variable. #679

Merged
merged 2 commits into from
May 27, 2022

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Jan 25, 2022

There are many use case where users want to know the current notebook
name/path. This help by adding a session identifier (to not really say
this is the current notebook name), and by default make it the full path
to the notebook document that created the session.

This will of course not work if the notebook get renamed, but we can
tackle this later.

See also jupyter/jupyter_client#656,
jupyter/notebook#6180. It will need to be ported
to jupyter_server as well.

This is the mirror commit of
jupyter/notebook#6279 on main notebook.

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2022

Codecov Report

Merging #679 (7abce6b) into main (af1ab9a) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #679      +/-   ##
==========================================
- Coverage   77.81%   77.80%   -0.01%     
==========================================
  Files         110      110              
  Lines       10415    10464      +49     
  Branches     1403     1404       +1     
==========================================
+ Hits         8104     8142      +38     
- Misses       1922     1930       +8     
- Partials      389      392       +3     
Impacted Files Coverage Δ
jupyter_server/services/sessions/sessionmanager.py 87.50% <ø> (ø)
jupyter_server/pytest_plugin.py 84.16% <0.00%> (-1.68%) ⬇️
jupyter_server/utils.py 65.90% <0.00%> (-0.57%) ⬇️
jupyter_server/services/contents/filemanager.py 70.22% <0.00%> (-0.34%) ⬇️
jupyter_server/tests/services/kernels/test_cull.py 100.00% <0.00%> (ø)
jupyter_server/tests/services/sessions/test_api.py 96.61% <0.00%> (+0.02%) ⬆️
jupyter_server/tests/services/kernels/test_api.py 95.13% <0.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af1ab9a...7abce6b. Read the comment docs.

@Wh1isper
Copy link
Contributor

The use of session should solve this problem, as far as I know the name of the session can be used to record the path

@Carreau
Copy link
Contributor Author

Carreau commented Jan 26, 2022

The use of session should solve this problem, as far as I know the name of the session can be used to record the path

I'm highly confused by this statement, and what it's trying to convey.
Yes session are the right things, and that's why this PR modify the session manager, and yes I use the session to get the path of the notebook. But even if the session knew the path had change you can't update the environment after the kernel has started....

@Wh1isper
Copy link
Contributor

Wh1isper commented Jan 26, 2022

The use of session should solve this problem, as far as I know the name of the session can be used to record the path

I'm highly confused by this statement, and what it's trying to convey. Yes session are the right things, and that's why this PR modify the session manager, and yes I use the session to get the path of the notebook. But even if the session knew the path had change you can't update the environment after the kernel has started....

Oh I see, sorry for not looking at the question carefully, have you considered the effect of root_dir on this path?
The relative path represented by path might be enough?
root_dir has the role of change root, which even allows absolute paths to path, for example
root_dir = '/a'
path = '/b.ipynb'
actually kernel start at /a/b.ipynb, but for front-end and user is b,ipynb only

The current session maintenance for files is probably enough, deleting the session when the file is moved or deleted seems to solve all the problems

@Carreau
Copy link
Contributor Author

Carreau commented Jan 26, 2022

Oh I see, sorry for not looking at the question carefully, have you considered the effect of root_dir on this path?

Yes, but I haven't check, my understanding is that the notebook server process CWD is root_dir.
I'm find with absolute or relative path in this env variable, as anyway it could be inconsistent.

@Wh1isper
Copy link
Contributor

Oh I see, sorry for not looking at the question carefully, have you considered the effect of root_dir on this path?

Yes, but I haven't check, my understanding is that the notebook server process CWD is root_dir. I'm find with absolute or relative path in this env variable, as anyway it could be inconsistent.

So it may expose the current server's root_dir configuration, and I'm not sure if that's appropriate, especially in jupyterhub, where users have some special security needs and configurations

There are many use case where users want to know the current notebook
name/path. This help by adding a session identifier (to not really say
this is the current notebook name), and by default make it the full path
to the notebook document that created the session.

This will of course not work if the notebook get renamed, but we can
tackle this later.

See also jupyter/jupyter_client#656,
jupyter/notebook#6180. It will need to be ported
to jupyter_server as well.

This is the mirror commit of
jupyter/notebook#6279 on main notebook.
@Carreau
Copy link
Contributor Author

Carreau commented Jan 26, 2022

Sure, removed the resolve().

@maartenbreddels
Copy link
Contributor

Love this PR, I think this is really useful!
In voila we've followed the CGI spec, see:

Maybe it is useful to have this standard in the Jupyter ecosystem, what do you think?

@echarles
Copy link
Member

What about multiple notebooks connected to a single Kernel?

e.g. in jupyterlab, you can connect multiple notebook, console (console don't have a path) to a single kernel.

What is the intended behavior? Should the JPY_SESSION_NAME be a list that can be updated?

@Carreau
Copy link
Contributor Author

Carreau commented Jan 26, 2022

It can't be a list, as it is an env variable, it could be a ; delimited string, sure.
In general I think we should be able to piggyback on the execution request, and for each execution request, say this execution comes from file/frontend XXX. Using for example user_expressions. But as explain in other linked thread, it might take a while to go there.

I'm fine with SCRIPT_NAME as well, or any other name that would get this in, I does not particularly matter to me, but we've been through the past few month/years through various proposal, and roughly:

  • there might not be a file/script, so having NOTEBOOK, FILE, SCRIPT... in the name was frowned upon.
  • JPY_ as a prefix seem to be common in Jupyter.

Now my idea is to already get that somehow, somewhere, as one source for the current file name or whatever, and at least in IPykernel, use this (and/or other sources), to set __file__ for each execution requests.

@echarles
Copy link
Member

It can't be a list, as it is an env variable, it could be a ; delimited string, sure.

yes, this is what I was meaning. However, thinking more, the maintenance of that string would be a pain when notebooks/console/files would connect/disconnect.

In general I think we should be able to piggyback on the execution request, and for each execution request, say this execution comes from file/frontend XXX. Using for example user_expressions. But as explain in other linked thread, it might take a while to go there.

That sounds like a good practice to identity the origin of the kernel message request. IIRC There is already a (unused) username field, not sure if there is a origin.

.... and at least in IPykernel, use this (and/or other sources), to set file for each execution requests.

this would work if the content if collocated to the kernel (same machine, same env), but would not be applicable for remote kernels or remote content (when the content is not colocated with the kernel).

@Carreau
Copy link
Contributor Author

Carreau commented Jan 26, 2022

this would work if the content if collocated to the kernel (same machine, same env), but would not be applicable for remote kernels or remote content (when the content is not colocated with the kernel).

Of course, unless you know how to map from one to the other.

@echarles
Copy link
Member

We have discussed this during the weekly server dev meeting:

  • @vidartf has suggested that we could consider the environment variable (as proposed in this PR) more as a useful hint that can be used (or not) by the kernel and the notebook user.
  • @blink1073 has suggested that a second step (enhancement outside of this PR) could be adding the source/origin of the kernel message as metadata to that message. This would not request a JEP. Like the debugger case, later on if we would want to move those details to a standard field, we could have a JEP process.

@echarles
Copy link
Member

I have relaunched the failing CI (Jupyter Server Tests on windows and Mac OS).

It is still failing with the same error. e.g on Windows jupyter_server/tests/services/sessions/test_api.py::test_restart_kernel[jp_server_config0]

  • Error: Process completed with exit code 1.

Is this expected due to some flakyness?

@blink1073
Copy link
Contributor

Yes, we've had some flakiness overall in CI recently.

@echarles
Copy link
Member

Based on the previous discussion, I think we agree to to move forward with this partial solution, and agree that it is already useful a a hint. @Carreau Maybe you could add in the doc a paragraph around this, open a subsequent issue to keep track of the remaining work. Based on that, we could merge. Thx again.

@vidartf
Copy link
Member

vidartf commented Mar 17, 2022

This will get merged tomorrow unless there are any more comments or suggestions!

@blink1073
Copy link
Contributor

Kicking CI since a lot has changed since the last commit.

@blink1073 blink1073 closed this Mar 17, 2022
@blink1073 blink1073 reopened this Mar 17, 2022
@blink1073
Copy link
Contributor

Whoa, this is new, on the Windows builds.

Fatal Python error: _Py_HashRandomization_Init: failed to get random numbers to initialize Python
Python runtime state: preinitialized

I just kicked a previously passing build to see if it happens there.

@blink1073
Copy link
Contributor

The other builds passed, kicked this one again.

@blink1073
Copy link
Contributor

Ah, I think what it is is that we are overriding the default env that is passed down here, which is preventing the random number generator from working on Windows.

@vidartf vidartf merged commit c8cbb2c into jupyter-server:main May 27, 2022
@vidartf
Copy link
Member

vidartf commented May 27, 2022

Merged as failing Windows tests seem to be unrelated.

@vidartf
Copy link
Member

vidartf commented May 27, 2022

Actually, they probably were relevant as @blink1073 indicated (why only Windows though?). I conflated it with #672

blink1073 pushed a commit that referenced this pull request Jul 13, 2022
)

* Inject session identifier into environment variable.

There are many use case where users want to know the current notebook
name/path. This help by adding a session identifier (to not really say
this is the current notebook name), and by default make it the full path
to the notebook document that created the session.

This will of course not work if the notebook get renamed, but we can
tackle this later.

See also jupyter/jupyter_client#656,
jupyter/notebook#6180. It will need to be ported
to jupyter_server as well.

This is the mirror commit of
jupyter/notebook#6279 on main notebook.

* Make start_kernel env extend os.environ (#859)

* Make start_kernel env extend os.environ

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Co-authored-by: Matthias Bussonnier <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

8 participants