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

Enable REPL to offer to install missing packages if install hooks are provided #39026

Merged
merged 5 commits into from
Apr 27, 2021

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Dec 29, 2020

Just trying out the ideas discussed in #26469 and in #35062 and the suggestion here #26469 (comment) to introduce a specific error type to allow other handlers to decide what to do.

This goes for a more basic y/n prompt than the example in #26469, with the default being n, which is selected here first.
It also leaves it up to the user to decide if the active environment is appropriate to install into.
A ctrl-c acts the same as returning n or just pressing enter.

Here the default is selected first by just hitting enter

asciicast

cc. @KristofferC @oxinabox

@IanButterworth IanButterworth force-pushed the ib/offer_to_install branch 2 times, most recently from a6aecac to 29ec192 Compare December 29, 2020 09:44
@KristofferC
Copy link
Member

Nice! I do think that Pkg itself needs to manage some of this though (and not only REPL). Some comments:

@oxinabox
Copy link
Contributor

I think the auto complete should only be done if the package exists in one of the current registries.

Strongly agreed. It would be annoying to get this if you had made a typo in the package name.
and so spent several seconds of it resolving only to get an error message..

It would be good to be able to enable auto installation of future packages (without requiring a prompt).

I think that can be a seperate PR?
This is useful without it, and the complexity of dealing with how to store persistient settings related to Pkg seems like it would bog this down.
Unless you just mean an eviroment variable that is set externally?

@IanButterworth
Copy link
Member Author

IanButterworth commented Dec 29, 2020

Ok, I've made some changes:

  • Switched to a similar hooks method to allow packages to hook into code loading #26469 but the difference is it hooks into when ModuleNotFoundError is thrown during REPL eval, rather than at the Base.require level
  • Added the appropriate Pkg hook in Add REPL hook to prompt to install missing packages, if available in registry Pkg.jl#2307
  • Checks for whether the package is available in installed registries. If so, prompts to install, if not, throws the old error directly
  • Re-evaluating the ast after the hook is finished, if it returns true, so that the package is loaded at the end. I thought this makes sense given that if the prompted install process completes without error, it would be weird if the original instruction wasn't successful

Demo: https://asciinema.org/a/adqDA5jSp3fusjSdIPOtttvqK

julia> using CSVV
ERROR: ModuleNotFoundError: Package CSVV not found in current path:
- Run `import Pkg; Pkg.add("CSVV")` to install the mod package.

Stacktrace:
 [1] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:867

julia> using CSV
 └ Package "CSV" not found, but "CSV" is listed in registry.
   Active project: /Users/ian/Documents/GitHub/Pkg.jl/Project.toml
   Add "CSV" to the active project? (y/n) [n]: y
   Resolving package versions...
Updating `~/Documents/GitHub/Pkg.jl/Project.toml`
  [336ed68f] + CSV v0.8.2
Updating `~/Documents/GitHub/Pkg.jl/Manifest.toml`
  [336ed68f] + CSV v0.8.2
  [e2d170a0] + DataValueInterfaces v1.0.0
  [82899510] + IteratorInterfaceExtensions v1.0.0
  [69de0a69] + Parsers v1.0.15
  [2dfb63ee] + PooledArrays v0.5.3
  [91c51154] + SentinelArrays v1.2.16
  [3783bdb8] + TableTraits v1.0.0
  [bd369af6] + Tables v1.2.2
  Progress [========================================>]  9/9
9 dependencies successfully precompiled in 11 seconds (37 already precompiled)

julia> CSV
CSV

@IanButterworth IanButterworth changed the title Enable REPL to offer to install missing packages, via new ModuleNotFoundError and handler Enable REPL to offer to install missing packages, via new ModuleNotFoundError and install hooks for Pkg etc to provide Dec 29, 2020
@DilumAluthge
Copy link
Member

This is only in interactive usage, right? I.e. in scripts, you'd just throw the error.

@IanButterworth
Copy link
Member Author

IanButterworth commented Dec 31, 2020

Yeah. That's the intention. This is what I've tested

ian@ian julia % ./julia -e "using CSV"
ERROR: ModuleNotFoundError: Package CSV not found in current path:
- Run `import Pkg; Pkg.add("CSV")` to install the mod package.

Stacktrace:
 [1] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:867

@IanButterworth
Copy link
Member Author

IanButterworth commented Jan 2, 2021

Just pushed to the corresponding pkg PR to improve the info and formatting of the prompt:
Screen Shot 2021-01-02 at 12 33 36 AM

Demo: https://asciinema.org/a/kO5Q4sZJp6qfI0a1Gtvo0xArx

@carstenbauer
Copy link
Member

I like this! However, do we need to force the user into a yes/no dialog? These can be rather annoying. What about a more minimal approach, something like

julia> using CSV
Package "CSV" not found, but available from a registry.
Install the package with `] add CSV`  or by pressing `CTRL+I`.

julia>

Which would install the package upon pressing CTRL+I right after such a message has occured.

@IanButterworth
Copy link
Member Author

One option is to make the default y, with a short timeout of a few seconds. I opened #39027 if that was desired.

That would feel less like forcing the user into a decision.

Providing a magic key combo to run the package add is also interesting

@StefanKarpinski
Copy link
Member

The yes/no dialog is what most package managers do here, so it seems pretty standard and easy.

@IanButterworth
Copy link
Member Author

IanButterworth commented Jan 15, 2021

Any further thoughts on this?

In summary I think the flow works well, and it seems like it would allow exotic front ends to do whatever it prefers when the ModuleNotFoundError is thrown.

However, I think there's one (hopefully small) con with this strategy of intercepting errors in the REPL evaluation..
Say the user does these examples, where Bar isn't installed

julia> using Foo, Bar

then Foo will be loaded twice, the 2nd time when the entire line is re-evaluated. I guess packages are usually safe to load twice though.

or

julia> x = 1;
julia> x += 1; using Bar

x would end up at 3.
That wouldn't be the case with

julia> x = 1;
julia> using Bar; x += 1

@IanButterworth
Copy link
Member Author

Just rebased and made this prompt only trigger for lines starting with using or import to avoid the edge case of re-running code on the same line that precedes the load, shown in the above comment.

Also added "and load" to the end to make it clearer that the package is loaded after install

image

function handle_install_package_hooks(mod, ast, backend)
line = string(ast.args[2])
# only exercise hooks for lines starting with `using/import` to avoid re-running non-load code
if startswith(line, "using") || startswith(line, "import")
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is the best way to check this, since there could also be leading whitespace or the using statement could be further nested but still run first. We could do some analysis on the parsed AST, but I am wondering if in practice just always reevaluating the whole input would really be a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

but I am wondering if in practice just always reevaluating the whole input would really be a problem.

Yeah, I think you might be right. I've removed the check for using/import

@miguelraz
Copy link
Contributor

Feature request: if a user does

using Foo, Bar

It will prompt to install all of them. As Stefan says, if users want an in between, they can do it manually.

@JeffBezanson
Copy link
Member

Suggestion from Stefan: After the repl input is parsed, search for using and import statements in the AST, collect all the names of packages not in the environment, and prompt to install all of them before doing eval.

@JeffBezanson
Copy link
Member

The advantage of Stefan's parse-first method is it avoids all issues with when and how many times things are evaluated. Even if the input contains if false; using X; end, prompting to install X still makes sense, and the conditional using still works, since all we've done is modify the project, not the state of the running process.

A "disadvantage" (but depends on your point of view) is that meta-programmed using statements will not prompt, but most of us in triage felt that case doesn't matter.

@JeffBezanson JeffBezanson added the forget me not PRs that one wants to make sure aren't forgotten label Apr 22, 2021
@IanButterworth
Copy link
Member Author

IanButterworth commented Apr 24, 2021

Now updated along with JuliaLang/Pkg.jl#2307 to @StefanKarpinski's suggestion of checking for all packages that will be loaded in the ast, then handling all packages in one act.

It seems to work quite nicely.

A single package

julia> using ImageIO
 │ Package "ImageIO" not found, but "ImageIO" is available from a registry.
 │ Project `~/.julia/environments/v1.7/Project.toml`
 └ Add "ImageIO" to the active project and load? (y/n) [n]: n
ERROR: ArgumentError: Package ImageIO not found in current path:
- Run `import Pkg; Pkg.add("ImageIO")` to install the ImageIO package.

Stacktrace:
 [1] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:873

Where all packages are missing, but only two packages are available in a registry

julia> using ImageIO, TiffImages, DoesNotExist
 │ Packages ["ImageIO", "TiffImages", "DoesNotExist"] not found, but ["ImageIO", "TiffImages"] are available from a registry.
 │ Project `~/.julia/environments/v1.7/Project.toml`
 └ Add ["ImageIO", "TiffImages"] to the active project and load? (y/n) [n]: n
ERROR: ArgumentError: Package ImageIO not found in current path:
- Run `import Pkg; Pkg.add("ImageIO")` to install the ImageIO package.

Stacktrace:
 [1] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:871

Where specific imports are declared

julia> using ImageIO: save
 │ Package ["ImageIO"] not found, but ["ImageIO"] is available from a registry.
 │ Project `~/.julia/environments/v1.7/Project.toml`
 └ Add ["ImageIO"] to the active project and load? (y/n) [n]: n
ERROR: ArgumentError: Package ImageIO not found in current path:
- Run `import Pkg; Pkg.add("ImageIO")` to install the ImageIO package.

Stacktrace:
 [1] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:871

In guarded code

julia> if false if false using ImageIO, TiffImages, DoesNotExist end end
 │ Packages ["ImageIO", "TiffImages", "DoesNotExist"] not found, but ["ImageIO", "TiffImages"] are available from a registry.
 │ Project `~/.julia/environments/v1.7/Project.toml`
 └ Add ["ImageIO", "TiffImages"] to the active project and load? (y/n) [n]: n

The first examplle but selecting y

julia> using ImageIO
 │ Package "ImageIO" not found, but "ImageIO" is available from a registry.
 │ Project `/var/folders/_6/1yf6sj0950vcg4t91m9ltb5w0000gn/T/jl_U2GrdN/Project.toml`
 └ Add "ImageIO" to the active project and load? (y/n) [n]: y
    Updating registry at `~/.julia/registries/General`
    Updating git-repo `https://github.com/JuliaRegistries/General.git`
   Resolving package versions...
   Installed Netpbm ──── v1.0.2
   Installed PNGFiles ── v0.3.7
   Installed ImageCore ─ v0.9.0
    Updating `/private/var/folders/_6/1yf6sj0950vcg4t91m9ltb5w0000gn/T/jl_U2GrdN/Project.toml`
  [82e4d734] + ImageIO v0.5.3
    Updating `/private/var/folders/_6/1yf6sj0950vcg4t91m9ltb5w0000gn/T/jl_U2GrdN/Manifest.toml`
...
Precompiling project...
  40 dependencies successfully precompiled in 48 seconds (1 already precompiled)

julia> ImageIO
ImageIO

The second example but selecting y

julia> using ImageIO, TiffImages, DoesNotExist
 │ Packages ["ImageIO", "TiffImages", "DoesNotExist"] not found, but ["ImageIO", "TiffImages"] are available from a registry.
 │ Project `~/.julia/environments/v1.7/Project.toml`
 └ Add ["ImageIO", "TiffImages"] to the active project and load? (y/n) [n]: y
    Updating registry at `~/.julia/registries/General`
    Updating git-repo `https://github.com/JuliaRegistries/General.git`
   Resolving package versions...
   Installed Netpbm ───── v1.0.1
   Installed TiffImages ─ v0.3.1
    Updating `~/.julia/environments/v1.7/Project.toml`
  [82e4d734] + ImageIO v0.5.3
  [731e570b] + TiffImages v0.3.1
    Updating `~/.julia/environments/v1.7/Manifest.toml`
...
Precompiling project...
...
ERROR: ArgumentError: Package DoesNotExist not found in current path:
- Run `import Pkg; Pkg.add("DoesNotExist")` to install the DoesNotExist package.

Stacktrace:
 [1] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:873

What it actually looks like on MacOS
image

Doesn't add noticeable overhead to REPL eval

@StefanKarpinski
Copy link
Member

Beautiful!

@IanButterworth IanButterworth changed the title Enable REPL to offer to install missing packages, via new ModuleNotFoundError and install hooks for Pkg etc to provide Enable REPL to offer to install missing packages if install hooks are provided Apr 25, 2021
if first(arg.args) isa Symbol # i.e. `Foo`
push!(mods, first(arg.args))
else # i.e. `Foo: bar`
push!(mods, first(first(arg.args).args))
Copy link
Member

Choose a reason for hiding this comment

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

I think this might need to be made more robust. I don't think this works for something like import A.b as c yet. Does this currently know to properly ignore imports like using .A?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think about either of those. I'll investigate and add tests

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the using .A case, and the other worked already. Added tests. Are there any other forms to watch out for?

function eval_with_backend(ast, backend::REPLBackendRef)
if !isempty(install_packages_hooks)
check_for_missing_packages_and_run_hooks(ast)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably run in the backend. It could even use the existing repl_ast_transforms hook (not a "transform" per se, but no big deal).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. Moved to the backend, but given that the modules_to_be_loaded parser resides here, it seemed like a misuse to place that on repl_ast_transforms given that if IJulia/Pluto etc. wanted to do a different install hook they'd probably want to go after the modules have been identified by modules_to_be_loaded so keeping install_packages_hooks would be necessary anyway. These use models are a bit imaginary though.. my thinking might be off

@JeffBezanson JeffBezanson merged commit df27063 into JuliaLang:master Apr 27, 2021
@IanButterworth IanButterworth deleted the ib/offer_to_install branch April 27, 2021 23:01
@IanButterworth
Copy link
Member Author

For those following this, there's some unresolved discussion about the message formatting in JuliaLang/Pkg.jl#2307
If anyone has any opinions it would be good to get that resolved & merged.

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
jarlebring pushed a commit to jarlebring/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
@simeonschaub simeonschaub removed the forget me not PRs that one wants to make sure aren't forgotten label May 29, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
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.

9 participants