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

[WIP] Use preferences.jl to set python executable #945

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PhilipVinc
Copy link
Contributor

@PhilipVinc PhilipVinc commented Dec 5, 2021

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
Copy link
Contributor Author

JuliaLang/Pkg.jl#2500

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2021

Codecov Report

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

Impacted file tree graph

@@            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%) ⬇️

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%) ⬇️
src/startup_helpers.jl 75.00% <ø> (ø)
src/startup.jl 46.15% <66.66%> (-6.98%) ⬇️
src/pyinit.jl 84.09% <100.00%> (+0.41%) ⬆️
src/pyeval.jl 70.27% <0.00%> (-1.81%) ⬇️
src/pybuffer.jl 60.67% <0.00%> (-0.44%) ⬇️
src/conversions.jl 63.08% <0.00%> (-0.25%) ⬇️
... 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
Copy link
Contributor Author

PhilipVinc commented Dec 5, 2021

TODO:

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

@tkf
Copy link
Member

tkf commented Dec 6, 2021

  • 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
Copy link
Contributor Author

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

@stevengj
Copy link
Member

stevengj commented Dec 8, 2021

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

gitignore
remove artifact


remove build phase


start adapting CI


fix ci


fix ci


fix ci


fix ci


fix ci conda


fixes


fix conda ci


stop testing on 1.0


remove testers


fix


fix aot ci


fixes


dont test build


fixes


fix venv test


refactor PyPreferences


use env variable


pyprefs


project


fix


fixes


stuff test


fix conda


revert changes


fix


cng


fix python version


custom which to work in julia 1.6


make test work with python 2


fixup


docs
@PhilipVinc
Copy link
Contributor Author

PhilipVinc commented Jan 19, 2022

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
Copy link

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
Copy link
Contributor

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
Copy link

xukai92 commented Feb 28, 2022

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.

@PhilipVinc
Copy link
Contributor Author

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
Copy link
Contributor

MilesCranmer commented Jul 4, 2023

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: MilesCranmer/PySR#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.

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.

8 participants