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

Use jupyter kernel mechanism #172

Merged
merged 10 commits into from
Aug 23, 2020

Conversation

fcollonval
Copy link
Contributor

@fcollonval fcollonval commented Aug 21, 2020

The future kernel provider mechanism is not coming soon: jupyter-server/jupyter_server#112

But more and more tools (voila, papermill,...) rely on the jupyter kernelspec mechanism. And there is at least one issue on those tools due to trouble when dealing with conda environment:

So as there is a good chance people are editing a notebook before using those tools, this add an option kernelspec_path to install the kernel spec dynamically discovered by this extension in the specified target path.

TODO:

  • Auto test
  • Update README
  • Correct nb_conda_kernels list

@fcollonval fcollonval marked this pull request as ready for review August 21, 2020 12:57
@fcollonval
Copy link
Contributor Author

@mcg1969 This is ready for review.

Some comments:

  • For backward compatibility, the new parameter is disabled. But it may be worth activating it by default to install the environment at the user level.
  • I change the configuration file to use jupyter_config.py and JupyterApp. This allows to get the configuration loaded even for python -m nb_conda_kernels list; and so to return results coherent with the user configuration.
  • I discover that the tests on windows are actually failing due to test_config.py::test_configuration. But the CI job itself is not failing 😕

@mcg1969
Copy link
Collaborator

mcg1969 commented Aug 21, 2020

Thank you @fcollonval! I will review ASAP. But what should we do with the failing Windows tests?

@mcg1969
Copy link
Collaborator

mcg1969 commented Aug 21, 2020

@fcollonval any opinions about the hacky approach taken in #104? Do you think there is merit to implementing both of these?

@fcollonval
Copy link
Contributor Author

Thank you @fcollonval! I will review ASAP. But what should we do with the failing Windows tests?

The CI job got broken due to a unfortunate rename of the log file in 9797e82
I'll open another PR to fix this.

@fcollonval
Copy link
Contributor Author

fcollonval commented Aug 21, 2020

@fcollonval any opinions about the hacky approach taken in #104? Do you think there is merit to implementing both of these?

I look at it. But firstly as you mentioned in #104, this is not really a sustainable approach - there are side effects; in particular the removal of the patch at each jupyter_client update (users will get confused). And secondly, from my understanding it won't cover the following use case:

  • base environment with JupyterLab/Notebook and nb_conda_kernels.
  • papermill environment with papermill (no nb_conda_kernels)
  • voila environment with voila (no nb_conda_kernels)

So with #104 the conda environments for papermill or voila will not be aware of the other environments as only the jupyter_client within the base environment will be patched.

With this modification, it is possible to install the kernel at the system or user level. Therefore all environments will be visible from any environment.

@mcg1969
Copy link
Collaborator

mcg1969 commented Aug 21, 2020

Got it, I appreciate the explanation! It really does give your approach a good defense. And it justifies building a utility to enable this more easily. But this PR doesn't have to wait for that.

Copy link
Collaborator

@mcg1969 mcg1969 left a comment

Choose a reason for hiding this comment

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

Thank you!

@mcg1969 mcg1969 merged commit 3705f4e into anaconda:master Aug 23, 2020
@fcollonval fcollonval deleted the use-jupyter-kernel-mechanism branch August 23, 2020 08:14
@fcollonval fcollonval mentioned this pull request Sep 4, 2020
@fcollonval
Copy link
Contributor Author

from my understanding it won't cover the following use case:

* `base` environment with JupyterLab/Notebook and `nb_conda_kernels`.
* `papermill` environment with papermill (no nb_conda_kernels)
* `voila` environment with voila (no nb_conda_kernels)

Small feedback on the exposed use case. In fact, it is not working if nb_conda_kernels is not install in the voila and papermill environments. The need for nb_conda_kernels in the other environments arise from the RUNNER_COMMAND in the kernel arguments.

So, this is not an argument in favor of this approach.

@mcg1969
Copy link
Collaborator

mcg1969 commented Sep 7, 2020

Interesting. Can we improve upon your approach somehow?

@mcg1969
Copy link
Collaborator

mcg1969 commented Sep 7, 2020

I don't think it is unreasonable to expect nb_conda_kernels to be present in any environment intended to leverage kernels. After all, wouldn't the kernel provider mechanism need to be in every such environment, too?

@kevin-bates
Copy link

After all, wouldn't the kernel provider mechanism need to be in every such environment, too?

Only if the application has adopted its use. Kernel providers are essentially a replacement of jupyter_client with jupyter_kernel_mgmt and jupyter_protocol combined with the various custom provider packages built upon them. The only application I'm aware of that has a plan to adopt kernel providers at this time is jupyter_server in its 2.0 timeframe.

@mcg1969
Copy link
Collaborator

mcg1969 commented Sep 7, 2020

Right. But right now nb_conda_kernels is really a patch on jupyter_client so it would make sense that any tool using the latter now should also bring in the former. And if jupyter_client is replaced with a new library that is what these tools need to pull in.

@fcollonval
Copy link
Contributor Author

I don't think it is unreasonable to expect nb_conda_kernels to be present in any environment intended to leverage kernels. After all, wouldn't the kernel provider mechanism need to be in every such environment, too?

Yes I agree with you. If the purpose is to alter a tool using the kernels, it make sense to install it along side all such tools.

@fcollonval
Copy link
Contributor Author

Can we improve upon your approach somehow?

We could change the kernel args to set the full executable path. But I see problem ahead for such case; like side effect due to the conda environment not being properly activated.

So I would say the best practice is to install nb_conda_kernels in all environments that need to be aware of the other kernels.

@fcollonval
Copy link
Contributor Author

BTW This raises the question of handling the root environment.
If I go back to the use case, I activate the voila environment. I run the voilà command on a notebook that use the root environment. This means the code won't be executed in a properly activated root environment as it does not use the RUNNER_COMMAND.
So should I patch the code, to add the runner on the root environment kernelspec installation.

@mcg1969
Copy link
Collaborator

mcg1969 commented Sep 7, 2020

I'm pretty sure it uses RUNNER_COMMAND for the root environment if the running environment is not root. But let's check.

@fcollonval
Copy link
Contributor Author

@mcg1969
Copy link
Collaborator

mcg1969 commented Sep 8, 2020

But those lines are correct. If we are running a kernel that lives in the current prefix, we assume the environment is already activated, and we need to skip it. We might need to adjust the command to use absolute paths but we shouldn't use the full RUNNER_COMMAND.

@fcollonval
Copy link
Contributor Author

But those lines are correct. If we are running a kernel that lives in the current prefix, we assume the environment is already activated, and we need to skip it. We might need to adjust the command to use absolute paths but we shouldn't use the full RUNNER_COMMAND.

Yes they are correct for usage within notebook or JupyterLab. But in the exported kernelspec there:

https://github.com/Anaconda-Platform/nb_conda_kernels/blob/e4481a1878623542a33e0e3adad4aba689bc808c/nb_conda_kernels/manager.py#L251-L252

RUNNER_COMMAND should be in all kernel specs as the sys.prefix using those kernel specifications may not be the one of the notebook/jupyterlab server.

Back to the use case, if I run voila on a notebook using the root environment kernel, the commands stack will be:

conda activate voila
voila notebook-using-root-env.ipynb

=> So the kernel will be executed with the root environment python. But the root environment will not be activated. So there may be inconsistency.

As proof from my computer, here is the content of $HOME/.local/share/jupyter/kernels/conda-root-py/kernel.json:

{"argv": ["/home/fcollonval/miniconda3/bin/python", "-m", "ipykernel_launcher", "-f", "{connection_file}"], "display_name": "Python [conda env:root] *", "language": "python", "metadata": {"conda_env_name": "root", "conda_env_path": "/home/fcollonval/miniconda3"}}

and the one for $HOME/.local/share/jupyter/kernels/conda-env-voila-py/kernel.json:

{"argv": ["python", "-m", "nb_conda_kernels.runner", "/home/fcollonval/miniconda3", "/home/fcollonval/miniconda3/envs/voila", "/home/fcollonval/miniconda3/envs/voila/bin/python", "-m", "ipykernel_launcher", "-f", "{connection_file}"], "display_name": "Python [conda env:voila]", "language": "python", "metadata": {"conda_env_name": "voila", "conda_env_path": "/home/fcollonval/miniconda3/envs/voila"}}

@mcg1969
Copy link
Collaborator

mcg1969 commented Sep 9, 2020

I see what you're saying, and I agree the behavior has to change for the exported case. I do want to make sure we do not change the activation behavior for the normal case, but that seems doable.

@mcg1969
Copy link
Collaborator

mcg1969 commented Sep 9, 2020

And actually, perhapsnb_conda_kernels.runner could also be modified not to re-activate if you're already in the correct environment.

@fcollonval
Copy link
Contributor Author

I will open a new issue to address that point.

I do want to make sure we do not change the activation behavior for the normal case, but that seems doable.

I agree too. It works nicely - let's not mess it up 😉

And actually, perhapsnb_conda_kernels.runner could also be modified not to re-activate if you're already in the correct environment.

Good suggestion, I'll try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants