-
-
Notifications
You must be signed in to change notification settings - Fork 423
change from using a build script to auto installing kernels and providing a jupyter path update function #1215
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
base: master
Are you sure you want to change the base?
Conversation
…ding a jupyter path update function The Pkg build functionality is not good. It gets run whenever a package is *downloaded* (which is a weird thing to trigger on where packages are considered immutable and multiple environments share the same package downloads), it requires the package to recompile when build options are changed etc. This instead auto installs the kernel if needed and also provides a separate function to update the jupyter path. It also moves to using the Preference system over using `DEPOT_PATH[1]/prefs` (but reads the old file as a fallback) It also changes some things that had to be done at build time (like setting the debug mode) to now just be a runtime thing
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1215 +/- ##
==========================================
- Coverage 70.21% 68.84% -1.38%
==========================================
Files 17 19 +2
Lines 1276 1409 +133
==========================================
+ Hits 896 970 +74
- Misses 380 439 +59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JamesWrigley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/src/manual/installation.md
Outdated
|
|
||
| !!! important | ||
| `Pkg.build("IJulia")` **must** be run at the Julia command line. | ||
| `IJulia.installkernel()` **must** be run in the Julia REPL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the whole admonition can be deleted now right?
docs/src/_changelog.md
Outdated
| patch release of Julia, but it does mean that IJulia will only create kernels | ||
| for each Julia minor release instead of each patch release. | ||
|
|
||
| ### Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be in a new 1.33.0 section.
src/config.jl
Outdated
| @@ -0,0 +1,112 @@ | |||
| using Scratch | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in thinking that we want to use Scratch here instead of Preferences because we don't want these settings to be tied to precompilation? Also, let's change this to using Scratch: ... to avoid implicit imports.
| using Scratch | |
| using Scratch: @get_scratch! |
src/config.jl
Outdated
| # Returns the stored Jupyter path, or empty string if not set. | ||
| function load_jupyter_preference() | ||
| # Check new Scratch.jl location first | ||
| prefsfile = joinpath(@get_scratch!("prefs"), "jupyter") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be @get_scratch!("jupyter")? IIUC we don't need the extra prefs directory with Scratch.
src/IJulia.jl
Outdated
| """ | ||
| module IJulia | ||
| export notebook, jupyterlab, installkernel | ||
| export notebook, jupyterlab, installkernel, update_jupyter_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not export update_jupyter_path, I expect it's less used than the other functions.
src/config.jl
Outdated
| # Use Conda Jupyter (empty string triggers auto-detection) | ||
| IJulia.update_jupyter_path("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would delete this in favour of having just one way of triggering auto-detection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean only ENV["JUPYTER"] = "" for auto?
docs/src/_changelog.md
Outdated
| or update kernels, and [`update_jupyter_path()`](@ref) to configure the | ||
| Jupyter executable path. | ||
| - IJulia now uses [Scratch.jl](https://github.com/JuliaPackaging/Scratch.jl) to | ||
| store configuration preferences instead of writing to `DEPOT_PATH/prefs/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not Preferences.jl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, the existence of Preferences.jl completely slipped my mind while I was doing this. It does seem better, I will rework this PR to use it and see how it feels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that make changing these settings force re-precompilation? I don't think we need that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They only force re-precompilation if the preferences are read during precompile time. I don't think they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually... would it be too much of a pain to go back to Scratch.jl? The reason is because I want to add some internal instrumentation (basically @trace_compile) that can be turned on or off, and environment variables don't really work for that since I don't always control the environment the kernel process is launched in. So I'd like to have some persistent setting that doesn't hook into precompilation, which sounds like Scratch.jl.
docs/src/_changelog.md
Outdated
|
|
||
| ### Changed | ||
| - Removed `Pkg.build("IJulia")` support. IJulia no longer uses a | ||
| build step for kernel installation. The build-time configuration system has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we still call installkernel() in a build.jl script, so that an external jupyter (or similar) front-end will immediately work after running add IJulia?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could but if add IJulia runs the build script or not is subject to the state of the file system. If the IJulia version is found in some existing DEPOT it won't build. So it feels kind of brittle to rely on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to re-run installkernel() if it was already run for that Julia version … what about running it during precompilation, which gets triggered on Pkg.add if you add to a new Julia version, even if the IJulia version was already present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about running it during precompilation, which gets triggered on Pkg.add if you add to a new Julia version, even if the IJulia version was already present?
My point is sort of that if you have set up IJulia at some point in the past, and then say removed all kernels and reinstalled conda etc, completely botched your existing IJulia setup, IJulia will not necessarily precompile nor build when you run add IJulia.
|
(This still won't fix external |
|
Just FYI, while doing this I was not aware of the juliaup issue being referenced (and this PR was not meant to address that) but from the comment I can sort of imagine what the problem is. Pkg Apps have the same problem in that we store the absolute path to the Julia executable that installed the path. When juliaup does GC that exe, it's sad times. My plan to help with the Pkg App problem is by using JuliaLang/juliaup#1315 in combination with a Pkg App specific env variable to override the executable that launch the app to be just |
|
Changed to using Preferences and did some updates based on review and own reading of code. I think the |
The Pkg build functionality was made for a different era in Julia package management. It gets run whenever a package is downloaded (which is a weird thing to trigger on where packages are considered immutable and multiple environments share the same package downloads), it requires the package to recompile when build options are changed etc. There are now better options based on e.g. scratch spaces.
Instead of using
Pkg.buildto install the default kernel, this instead auto installs it if needed.It provides a separate function to update the jupyter path because tying kernel installation and updating of jupyter path together felt wrong.
It moves to using the Preference system over using
DEPOT_PATH[1]/prefs(but reads the old file as a fallback)It changes some things that previously had to be done at build time (like setting the debug mode) to now just be a runtime thing.
Using IJulia from scratch (by using an empty depot and clearing out all kernels) looks like: