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

Set JULIA_PROJECT, use Pkg.add once #186

Merged
merged 5 commits into from
Sep 6, 2022

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Sep 6, 2022

  • Sets JULIA_PROJECT before loading pyjulia so that PyCall.jl can be contained within the pysr environment
  • Also use Pkg.add in a single step to add both SymbolicRegression.jl and ClusterManagers.jl to the environment at the same time

I likely advised against using the environment variable JULIA_PROJECT in the past. However, I think this may be necessary to avoid interference from other projects if installed within the same environment.

@MilesCranmer
Copy link
Owner

Thanks! Just to check, this would still allow a user to set a custom julia_project in the PySRRegressor, right?

@MilesCranmer
Copy link
Owner

Just tested it, and it seems to work. Cool!

@MilesCranmer MilesCranmer merged commit 52a6b5b into MilesCranmer:master Sep 6, 2022
@MilesCranmer
Copy link
Owner

Btw, do you know how one could restart PyJulia? Right now (even before this PR), you can't change the environment after the first call to .fit, or the number of threads. Maybe there is an easy way to restart PyJulia to change these things, and load using the new environment variables.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 6, 2022

Yes. Sorry, I got distracted. Thanks for merging.

Overall, this should provide greater flexibility in terms of where PyCall.jl is installed.

@MilesCranmer
Copy link
Owner

Thanks. Not sure you saw this message, but do know if there is a way to restart PyJulia?

Also - I just updated PyPI to 0.10.2 with this merge - so feel free to update the conda-forge PR as it will now have your JULIA_PROJECT change.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 6, 2022

Btw, do you know how one could restart PyJulia? Right now (even before this PR), you can't change the environment after the first call to .fit, or the number of threads. Maybe there is an easy way to restart PyJulia to change these things, and load using the new environment variables.

Not really, although you can kind of simulate it using addprocs, which I think you kind of figured out. I'm not sure if I understand about not being able to change the environment. Certainly you can change Julia project environments via Pkg.activate.

Perhaps you mean changing environment variables such as JULIA_NUM_THREADS to increase the number of the threads?

I recall Jeff Bezanson talking about interactive threads in the State of Julia 2022 talk:
https://youtu.be/N4h46_TCmGc?t=962 (see minute 16:00)

@mkitti
Copy link
Contributor Author

mkitti commented Sep 6, 2022

If it is not too much trouble, could you also create a Github "release" for 0.10.1 and 0.10.2?

One future change I am contemplating is whether there should be a PYSR_PROJECT environment variable that could act as a default if julia_project is None. I need to think more about how that might work with distributed computing.

@MilesCranmer
Copy link
Owner

If it is not too much trouble, could you also create a Github "release" for 0.10.1 and 0.10.2?

Done!

One future change I am contemplating is whether there should be a PYSR_PROJECT environment variable that could act as a default if julia_project is None. I need to think more about how that might work with distributed computing.

I think that is a good idea. For distributed computing I don't think it should be an issue, since the processes are all launched from within Julia, and then activated using the current project.

@MilesCranmer
Copy link
Owner

Do you know what these errors are caused by? https://github.com/MilesCranmer/PySR/actions/runs/3006737521 It looks like they are new - not sure what from. Specifically they are only on macOS/windows on Julia <= 1.6

@mkitti
Copy link
Contributor Author

mkitti commented Sep 7, 2022

Those are likely related to my changes. Essentially PyCall has somehow failed to be installed in the current environment or we failed to change environments.

@mkitti
Copy link
Contributor Author

mkitti commented Sep 7, 2022

Could you point me to the code for the tests that are failing?

@MilesCranmer
Copy link
Owner

I think it's failing at the install step of these tests:

python -c 'import pysr; pysr.install()'

@mkitti
Copy link
Contributor Author

mkitti commented Sep 7, 2022

Here's my guess at the moment. As you found out before, not all of the Julia API recognizes that @ indicates a shared environment. The tests that are failing are on Julia versions that think JULIA_PROJECT=@pysr-0.10.3 indicates that the Julia environment exists in a local folder called @pysr-0.10.3 rather than ~/.julia/environments/pysr-0.10.3. However, with successive Julia versions this improved, which is why we do not see the issue on Julia 1.7 and beyond.

Some of the implementations then install PyCall in the local folder rather that the shared environment in the depot. But when they try to load PyCall they don't find it in the shared environment. This raises an ArgumentError similar the one I found yesterday in the conda-forge feedstock.

There is likely something else going on that accounts for the operating system dependence of the issue.

@MilesCranmer
Copy link
Owner

I see, thanks. So I guess we should add a method to revert to the old Pkg.activate if the julia version is under 1.7?

@MilesCranmer
Copy link
Owner

MilesCranmer commented Sep 7, 2022

If I run

from julia.core import JuliaInfo
info = JuliaInfo.load()

before I set JULIA_HOME, would it already load Julia?

This is to check the version beforehand.


Edit: Yep, it seems fine to run this beforehand.

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.

None yet

2 participants