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

Urgent: add option to disable numpy install / prevent corruption of conda-forge environments #1040

Merged
merged 3 commits into from Jun 16, 2023

Conversation

MilesCranmer
Copy link
Contributor

@MilesCranmer MilesCranmer commented Jun 12, 2023

Right now PyCall.jl automatically installs numpy into a conda environment. This install cannot be disabled. If this occurs from within an existing conda environment, it can overwrite the numpy installed by a conda-forge build. This violates the assumptions of conda-forge, and can silently corrupt any conda env that depends on numpy.

This issue was noticed in conda-forge/pysr-feedstock#85, manifesting in this issue: conda-forge/scipy-feedstock#238. We believe this is caused by the line:

Conda.add("numpy")

This PR enables the PYCALL_INSTALL_NUMPY environment flag which can be set to false to disable the Conda.add("numpy") line and prevent the corruption of a conda env. The default behavior is unchanged if this flag is not set, so this is backwards compatible.

cc @h-vetinari @mkitti @ngam

@MilesCranmer MilesCranmer changed the title Urgent: add option to disable numpy install (which can corrupt conda environment) Urgent: add option to disable numpy install / prevent corruption of conda-build environments Jun 12, 2023
@MilesCranmer MilesCranmer changed the title Urgent: add option to disable numpy install / prevent corruption of conda-build environments Urgent: add option to disable numpy install / prevent corruption of conda-forge environments Jun 12, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: +0.19 🎉

Comparison is base (bcaba00) 67.03% compared to head (e6f3744) 67.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1040      +/-   ##
==========================================
+ Coverage   67.03%   67.22%   +0.19%     
==========================================
  Files          20       20              
  Lines        2017     2017              
==========================================
+ Hits         1352     1356       +4     
+ Misses        665      661       -4     
Flag Coverage Δ
unittests 67.22% <ø> (+0.19%) ⬆️

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

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mkitti
Copy link
Member

mkitti commented Jun 12, 2023

I'm not sure I'm following. All PyCall.jl wants to do is ensure that the conda environment it is operating in has numpy installed. Normally, this is a conda environment that is created specifically for a Julia install, but from a conda installed Julia this is an existing conda environment.

@mkitti
Copy link
Member

mkitti commented Jun 12, 2023

Could we consider adding --freeze-installed instead?

@MilesCranmer
Copy link
Contributor Author

Wait, I thought this line was the cause of the downstream issues? Doesn’t Conda.add() update the numpy version and reinstall?

@MilesCranmer
Copy link
Contributor Author

https://docs.conda.io/projects/conda/en/latest/commands/install.html

Conda attempts to install the newest versions of the requested packages. To accomplish this, it may update some packages that are already installed, or install additional packages. To prevent existing packages from updating, use the --freeze-installed option.

(maybe that’s what you meant by --freeze-installed?)

@MilesCranmer
Copy link
Contributor Author

I think it’s safer to entirely turn off calls to conda install when inside a conda-forge build script though. Just feels a bit dangerous. What if even with freeze-installed, it installs some other packages which are not present in the env that conda-forge is building?

@mkitti
Copy link
Member

mkitti commented Jun 12, 2023

Actually, I think we want --satisfied-skip-solve

@mkitti
Copy link
Member

mkitti commented Jun 12, 2023

What if even with freeze-installed, it installs some other packages which are not present in the env that conda-forge is building?

It should throw errors since you did not previously add all the needed dependencies.

@MilesCranmer
Copy link
Contributor Author

Okay I added the required functionality in JuliaPy/Conda.jl#240

@mkitti
Copy link
Member

mkitti commented Jun 12, 2023

See JuliaPy/Conda.jl#241

@ngam
Copy link

ngam commented Jun 12, 2023

I think it’s safer to entirely turn off calls to conda install when inside a conda-forge build script though.

If this is possible, it is desirable. However, it likely should be done as a patch in conda-forge, not here. I will leave that up to you though. Why would PyCall need numpy to begin with? That’s kinda odd and arbitrary (isn’t it supposed to be a generic python caller?)

@stevengj
Copy link
Member

Why would PyCall need numpy to begin with? That’s kinda odd and arbitrary (isn’t it supposed to be a generic python caller?)

In order to wrap Julia arrays with numpy arrays when calling Python.

2 similar comments
@stevengj
Copy link
Member

Why would PyCall need numpy to begin with? That’s kinda odd and arbitrary (isn’t it supposed to be a generic python caller?)

In order to wrap Julia arrays with numpy arrays when calling Python.

@stevengj
Copy link
Member

Why would PyCall need numpy to begin with? That’s kinda odd and arbitrary (isn’t it supposed to be a generic python caller?)

In order to wrap Julia arrays with numpy arrays when calling Python.

@MilesCranmer
Copy link
Contributor Author

@mkitti Updated with satisfied_skip_solve=true

@mkitti mkitti requested a review from stevengj June 13, 2023 01:18
Copy link
Member

@mkitti mkitti left a comment

Choose a reason for hiding this comment

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

The revised build does not try to upgrade numpy if it is in the configured conda environment.

@mkitti
Copy link
Member

mkitti commented Jun 13, 2023

@stevengj could we merge and release this change?

@MilesCranmer
Copy link
Contributor Author

Just pinging this - could this be merged @stevengj?

@mkitti
Copy link
Member

mkitti commented Jun 14, 2023

I only have an auto-merge button here.

image

An chance we could get the other CI bits to pass? Python 2.7 should probably just be retired...

@MilesCranmer
Copy link
Contributor Author

It says:

Error: Version 2.7 with arch x64 not found

so we can't even test it. Also, pyjulia already dropped support.

@MilesCranmer
Copy link
Contributor Author

@stevengj sorry for the spam but we really need this fixed to solve some downstream failures. Any chance you could merge+release?

@ngam
Copy link

ngam commented Jun 16, 2023

@MilesCranmer, is pycall available in conda-forge somehow? What in conda-forge uses it? If that's the case, we could patch it and move the ball forward instead of waiting.

@ngam
Copy link

ngam commented Jun 16, 2023

Or is it the thing that Julia calls and we bundle in your pysr setup?

@ngam
Copy link

ngam commented Jun 16, 2023

If so, we could point your pysr package on Conda-forge to use this https://github.com/MilesCranmer/PyCall.jl/tree/patch-numpy-install (I don't know how that's done in Julia, but I assume it is easily doable, right?)

@MilesCranmer
Copy link
Contributor Author

Right, we bundle it with the PySR install as a special julia depot.

@stevengj stevengj merged commit b5da159 into JuliaPy:master Jun 16, 2023
15 of 18 checks passed
@MilesCranmer MilesCranmer deleted the patch-numpy-install branch June 16, 2023 20:38
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

5 participants