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

Unpin numpy as a host dependency #85

Merged
merged 10 commits into from
Jun 6, 2023
Merged

Conversation

MilesCranmer
Copy link
Contributor

Fixes #83

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member

Wait with merging this. I found another big issue here.

recipe/meta.yaml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member

Wait with merging this. I found another big issue here.

Please remove the two lines mentioned in #86

@MilesCranmer
Copy link
Contributor Author

#87 does this

@h-vetinari h-vetinari mentioned this pull request Jun 5, 2023
@h-vetinari
Copy link
Member

@conda-forge-admin, please rerender

@MilesCranmer
Copy link
Contributor Author

Are these warnings anything to worry about?

ClobberWarning: This transaction has incompatible packages due to a shared path.
  packages: conda-forge/linux-64::numpy-1.24.3-py310ha4c1d20_0, local/linux-64::pysr-0.14.1-py310hfdc917e_1
  path: 'lib/python3.10/site-packages/numpy/core/tests/test_dlpack.py'

Apart from that, things seem to be working. Are you happy with this being merged once tests pass?

@h-vetinari
Copy link
Member

Are these warnings anything to worry about?

Yeah, this is not a good thing and needs to be fixed. It looks like your installation process is reinstalling numpy, including some of the test sources, and this gets picked up by conda's before/after-snapshotting of the host environment. The answer should be simple: don't reinstall numpy, use the one that's installed in host already.

@MilesCranmer
Copy link
Contributor Author

It's likely this: 8fac64f (just pushed)

But, if not, there is a small chance it is from the PyCall.jl build process, which tries to install numpy if not already available: https://github.com/JuliaPy/PyCall.jl/blob/bcaba00d1e2c412b2f61d33343ef5a9ab1b9258a/deps/build.jl#L79.

@h-vetinari
Copy link
Member

Numpy is still being spuriously installed.

But, if not, there is a small chance it is from the PyCall.jl build process, which tries to install numpy if not already available:

I haven't been involved with the julia effort in conda-forge, but this must not happen. If it cannot be solved through a build argument, then we have to patch something here. @mkitti - any experience with this issue?

@MilesCranmer
Copy link
Contributor Author

@MilesCranmer
Copy link
Contributor Author

Are you sure that this warning message is problematic? It seems to only show that warning in the testing stage: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=717759&view=logs&j=d0d954b5-f111-5dc4-4d76-03b6c9d0cf7e&t=841356e0-85bb-57d8-dbbc-852e683d1642&l=798

Some of these tests involve re-building, and building in temporary environments:

- julia --project=@pysr-{{ version }} -e "using PyCall, SymbolicRegression, ClusterManagers"
- python -c 'from pysr.sr import init_julia; init_julia()'
- python -c 'import pysr; pysr.install(); from pysr.sr import init_julia; init_julia()'
- python -c 'import tempfile; tempenv = tempfile.mkdtemp(); import pysr; pysr.install(tempenv); from pysr.sr import init_julia; init_julia(tempenv)'

@mkitti
Copy link
Contributor

mkitti commented Jun 5, 2023

Umm... well numpy is a dependency of pysr, right? The julia-feedstock activate script should tell PyCall.jl and Conda.jl to look at the current conda environment. It should then realize that numpy is already installed, and not install a new one.

What is happening here?

@MilesCranmer
Copy link
Contributor Author

It could be that this line is checking for numpy in the root environment, rather than the environment we are building/testing PySR in?

i.e., here is the add:

        Conda.add("numpy")

and here is a note about the default environment:

The Conda.jl package supports environments by allowing you to pass an optional
env parameter to functions for package installation, update, and so on. If
this parameter is not specified, then the default "root" environment
(corresponding to the path in Conda.ROOTENV) is used. The environment name can
be specified as a Symbol, or the full path of the environment
(if you want to use an environment in a nonstandard directory) can
be passed as a string.

For example:

using Conda
Conda.add("libnetcdf", :my_env)
Conda.add("libnetcdf", "/path/to/directory")
Conda.add("libnetcdf", "/path/to/directory"; channel="anaconda")

But I still don't understand how this should affect things, since these warnings are only displayed at the test stage?

@h-vetinari
Copy link
Member

It's interesting that this warning doesn't occur on main despite seemingly nothing else in the build process changing.

There's a likely explanation: Before, you were using the newest numpy version (1.24.3) to build, which very likely matched whatever julia was pulling in. Now the build is using 1.21 to build, and so the file delta in $PREFIX1 between numpy 1.21 (the "before" in the snapshot) and numpy 1.24 (the "after" as installed by julia) is what conda now thinks are files installed by pysr.

Some of these tests involve re-building, and building in temporary environments:

The ClobberWarnings happen when the environment is created, which is before these test commands are ever run.

Footnotes

  1. more precisely, in $PREFIX/lib/python3<x>/site-packages/numpy, at least as far as the numpy installation is concerned

@MilesCranmer
Copy link
Contributor Author

Is there any "band-aid" fix I can apply to the build while I look into this out in the PyCall.jl side? (I don't suppose it would work as-is, would it?) Just something to fix it for users temporarily so they aren't stuck with this libstdc++ bug themselves.

Maybe I can temporarily pin libstdcxx-ng to the earlier version, finish off this PR, and then unpin it?

@h-vetinari
Copy link
Member

You have two choices, both user-unfriendly in some way:

  • keep building against numpy 1.24.3 (status quo before this PR) - which means all users of pysr need numpy>=1.24.3 at runtime, but the warnings are gone
  • build against 1.21 as in this PR currently (meaning users can use any numpy >=1.21 at runtime), and introduce pretty noisy (but in this particular case relatively harmless) ClobberWarnings.

Whatever choice you make to stop the bleeding now (which is fair enough), fixing this properly should be a high priority. Randomly re-installing numpy is baaaaaaaaaaaaaaaaaaaaaaad. Please don't do it.

recipe/meta.yaml Outdated
skip: true # [win]
requirements:
host:
- python
- pip
- pyjulia >=0.6.0
- julia
- numpy >=1.14.0
- numpy
Copy link
Member

Choose a reason for hiding this comment

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

If you want to go with option one, you should not revert the changes here, but additionally add the following:

Suggested change
- numpy
- numpy
# temporarily rebuilding with newest numpy because PyCall.jl ends up
# reinstalling it and we want to avoid spamming ClobberWarnings
# TODO: remove this as soon as numpy does not get spuriously installed here anymore
- numpy >=1.24.3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was the first numpy key a typo? Or did you mean to add it two times when you said "additionally add"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I think it was a typo; let me undo this...)

Copy link
Member

Choose a reason for hiding this comment

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

No, it was not a typo, that's exactly why I said "additionally".

One line (the unpinned one) is for being up correctly by our infrastructure (conda smithy etc.) as I explained above. The other line is a temporary, additional constraint.

Copy link
Member

Choose a reason for hiding this comment

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

I even emphasised additionally...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MilesCranmer
Copy link
Contributor Author

Thanks, that sounds good to me.

Whatever choice you make to stop the bleeding now (which is fair enough), fixing this properly should be a high priority. Randomly re-installing numpy is baaaaaaaaaaaaaaaaaaaaaaad. Please don't do it.

I agree! That this was even going on was news to me as well. Quite a deeply buried bug in the PyJulia infra. And I guess it makes sense we never saw any signal of this since we were building against the same numpy.

MilesCranmer and others added 2 commits June 6, 2023 10:27
Co-authored-by: h-vetinari <h.vetinari@gmx.com>
@MilesCranmer
Copy link
Contributor Author

@conda-forge-admin, please rerender

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.

Unpin numpy as a host-dependency
3 participants