[WIP] Use preferences.jl to set python executable

[PhilipVinc]

This PR is a re-exhumation of #835, and most of the credit goes to @tkf .

An internal package PyPreferences.jl exposes a simple API (use_system(), use_conda()) to set the current python, and detects version/pythonhome so that this code can be removed from the build phase of PyCall.

Since it stores those options using Preferences.jl, changing the python executable will trigger a recompilation. Unfortuantely, as Preferences.jl only works on Julia>=1.6, this means that either we leave in place the old build machinery for older Julia versions and switch to the new one only on recent Julia versions, or we drop support for Julia <1.6. This would involve considerable more work and would make testing more complex.

Is dropping support an option?

I'm going to put this up so that I can see how badly CI fails..

Another thing to consider: With respect to PkgCompiler.jl, do we want PyCall to support changing the executable path/version after compilation or that is not required?

[PhilipVinc]

https://github.com/JuliaLang/Pkg.jl/issues/2500

[codecov-commenter]

Codecov Report

Merging #945 (7c86096) into master (e3e3008) will decrease coverage by 0.52%. The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #945      +/-   ##
==========================================
- Coverage   68.33%   67.80%   -0.53%     
==========================================
  Files          20       21       +1     
  Lines        2018     2047      +29     
==========================================
+ Hits         1379     1388       +9     
- Misses        639      659      +20     

Flag	Coverage Δ
unittests	67.80% <80.00%> (-0.53%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/PyCall.jl	69.87% <ø> (-0.83%)	:arrow_down:
src/startup_helpers.jl	75.00% <ø> (ø)
src/startup.jl	46.15% <66.66%> (-6.98%)	:arrow_down:
src/pyinit.jl	84.09% <100.00%> (+0.41%)	:arrow_up:
src/pyeval.jl	70.27% <0.00%> (-1.81%)	:arrow_down:
src/pybuffer.jl	60.67% <0.00%> (-0.44%)	:arrow_down:
src/conversions.jl	63.08% <0.00%> (-0.25%)	:arrow_down:
... and 2 more

---

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 e3e3008...7c86096. Read the comment docs.

[PhilipVinc]

TODO:

• cleanup where things like find_libpython should live. PyPreferences or PyCall?
• PkgCompiler tests are broken but I need to figure out why.

[tkf]

• cleanup where things like find_libpython should live.

I'd suggest PyPreferences. I think it'd be nice if all the code currently in the deps is in some functions of PyPreferences. It makes testing and debugging easier. Maybe we can even put some troubleshooting UI in it in the future based on the libpython discovery code.

@stevengj What do you think? You seemed to prefer merging PyPreferences to PyCall instead, in #835.

[PhilipVinc]

I'd also like to know if dripping pre 1.6 compatibility is on the table.

[stevengj]

I think it's fine to drop 1.6 support going forward.

[PhilipVinc]

One problem I encountered is the following: Right now the preference for python can be set both to a command in the PATH or to a full path to an executable.

However, if an user changes the PATH, in order to get a different python version/venv on different folders (this happens to me using direnv) but the executable name stays the same, this will not trigger recompilation, however we would need to recompile in this case.

One thing that can be done is to normalise the python executable to a path before storing it in the preferences. However this does not completely avoid the problem: the user could still manually set the python executable python3 by hand in LocalPreferences.toml. Alternatively, we could add a _fullpath key to preferences, and warn users not to edit it, but that seems a bad approach.

A better way would be to add the hash of the current python executable path to the cache invalidation mechanism, so that if the executable changed we trigger a recompile, but I do not know if that is possible (maybe @StefanKarpinski who worked on this knows?)..

--

Lastly: I don't have access to a windows machine to test the failures of venv on windows. If somebody where to step in/help, it would be helpful. Also, I don't understand why the AOT tests are failing.

[StefanKarpinski]

I think @staticfloat would be the person to comment since he implemented Preferences (I consulted on design but didn't do any of the implementation).

[staticfloat]

Here's how I would address this:

When you save the preference out, you always normalize it to a full path. If the user edits a TOML to change the value to a PATH-relative basename, that will trigger recompilation, and while compiling (e.g. at the top level) you will normalize and save it out again. E.g. something like:

function save_python_path(path::String)
    if !isabspath(path)
        which_path = Sys.which(path)
        if which_path === nothing
            error("Couldn't find python executable at $(path)")
        end
    end
    @set_preferences!("PYTHON" => path)
end

python_path = @get_preference("PYTHON")
if !isabspath(python_path)
    save_python_path(python_path)
end

Base.include_dependency(python_path)

So usage might go something like this:

1. User installs package, compilation happens with default python path (whatever that is).
2. User sets the python path to python, which gets normalized and saved. Next time it is loaded it recompiles, but after that it's fine.
3. User modifies the saved preferences manually, which causes recompilation. During recompilation, the path gets normalized and saved again. Next time it is loaded, it does not recompile.

If you want to recompile if the python executable itself changes, the best we can do is to use Base.include_dependency(python_path), which will trigger recompilation if the mtime of that file changes from the last time we compiled.

[xukai92]

I was trying to test this PR but with no success---do I need a different version of pyjulia or do I mis-understand what this PR solves (I thought it's to fix the statically-linked Python issue)?

What I tried was dev this branch locally so that my julia picks up this PR and attempt to load a statically-linked Python. This gave me same error (the one suggests passing compiled_modules=False) as before when I using the master branch of PyCall.jl.

[MilesCranmer]

Ping. What's the status of this PR?

This is badly needed. Downstream, several users have ran into bugs in my packages because of this:

• https://github.com/MilesCranmer/PySR/discussions/262
• https://github.com/MilesCranmer/PySR/issues/287
• https://github.com/MilesCranmer/PySR/issues/257
• https://github.com/MilesCranmer/PySR/issues/222
• https://github.com/MilesCranmer/PySR/issues/140
• Attempts at downstream patches:
  • https://github.com/MilesCranmer/PySR/pull/365
  • https://github.com/MilesCranmer/PySR/pull/363

[PhilipVinc]

Im not working in Julia anymore unfortunately, as Jax has captured my attention and that of my colleagues.

Feel free to pick it up. I remember there being agreement in the fact that this should be done, but it was hard to get hold of the maintainers...

[MilesCranmer]

Sad to hear you left Julia (why not both 😉?) but thanks for updating me on the PR status.

For others needing something like this due to PyCall.jl build issues (e.g., if you change Python version) – you might be interested in a similar workaround to the one implemented in PySR: https://github.com/MilesCranmer/PySR/pull/363. Thanks @mkitti for the idea.

Basically:

import Pkg
Pkg.activate("pysr..."; shared=true)
ENV["PYTHON"] = "/path/to/python"
Pkg.build("PyCall")

will trigger PyCall to rebuild when you switch Python versions.

It doesn't fix things if you switch back and forth Python versions, but it's good enough for my target demographic who might upgrade Python once a year and wonder why PySR is breaking.