search

conda-forge / pysr-feedstock

A conda-smithy repository for pysr.
BSD 3-Clause "New" or "Revised" License
0 stars 6 forks source link

Unpin numpy as a host dependency #85

Closed MilesCranmer closed 1 year ago

MilesCranmer commented 1 year ago

Fixes #83

conda-forge-webservices[bot] commented 1 year ago

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 commented 1 year ago

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

h-vetinari commented 1 year ago

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

Please remove the two lines mentioned in #86

MilesCranmer commented 1 year ago

87 does this

h-vetinari commented 1 year ago

@conda-forge-admin, please rerender

MilesCranmer commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

It's likely this: https://github.com/conda-forge/pysr-feedstock/pull/85/commits/8fac64f0b804e97b77180e6534fd49d5217f47f0 (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 commented 1 year ago

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 commented 1 year ago

It's interesting that this warning doesn't occur on main: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=717729&view=logs&jobId=d0d954b5-f111-5dc4-4d76-03b6c9d0cf7e&j=d0d954b5-f111-5dc4-4d76-03b6c9d0cf7e&t=841356e0-85bb-57d8-dbbc-852e683d1642 despite seemingly nothing else in the build process changing.

MilesCranmer commented 1 year ago

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:

https://github.com/conda-forge/pysr-feedstock/blob/6b5be685bf409af2304d18b94181db87c7c90843/recipe/meta.yaml#L47-L50

mkitti commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 $PREFIX[^1] 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.

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

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.

MilesCranmer commented 1 year ago

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 commented 1 year ago

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

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.

MilesCranmer commented 1 year ago

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 commented 1 year ago

@conda-forge-admin, please rerender