easybuilders / easybuild-easyblocks

Collection of easyblocks that implement support for building and installing software with EasyBuild.
https://easybuild.io
GNU General Public License v2.0
106 stars 283 forks source link

FortranPythonPackage ignores use_pip #2518

Open Flamefire opened 3 years ago

Flamefire commented 3 years ago

E.g. EB_scipy uses FortranPythonPackage which ignores use_pip and always builds with setup.py It then uses pip to build the package again (this time without the Fortran settings) and installs that.

Not fully sure if that 2nd step reuses the stuff from the build step, but it looks odd at least

migueldiascosta commented 6 months ago

E.g. EB_scipy uses FortranPythonPackage which ignores use_pip and always builds with setup.py It then uses pip to build the package again (this time without the Fortran settings) and installs that.

Indeed

Not fully sure if that 2nd step reuses the stuff from the build step, but it looks odd at least

I don't think it reuses anything, it looks like we could put the at least the run_cmd in a if self.use_setup_py: and it would save build time without changing what is installed.

Additionally, when actually trying to use a different compiler with pip, passing --fcompiler to pip install via --build-option or --global-option as I had done before seems to be ignored now, and I also couldn't get it to work with --config-settings, the only way I could make pip actually use a different Fortran compiler was, inspired by spack (https://github.com/spack/spack/blob/fecb63843e90701667d4db7f07ef4ab9739bd985/var/spack/repos/builtin/packages/py-scipy/package.py#L179-L191), to add it to a [config_fc] section in setup.cfg

Flamefire commented 6 months ago

I don't think it reuses anything

I'm not sure under which conditions it reuses the build. For example for PyTorch, which we now install with pip instead of setup.py too, I added an setup.py build command in the build step such that the (long) build is done in the build step and not the test step which does a temporary install making it harder to tell what failed. But there the same/equivalent flags are used for both invocations

Additionally, when actually trying to use a different compiler with pip, passing --fcompiler to pip install via --build-option or --global-option

I have found this report in PIP upstream: https://github.com/pypa/pip/issues/6379#issue-428614617

correctly errors out due to intercepted option --fcompiler (non-installed compiler) git clone git+https://github.com/zerothi/sisl.git cd sisl pip install --global-option="build" --global-option="--fcompiler=nag" .

I also found QML docs

pip install qml --user -U --global-option="build" --global-option="--compiler=intelem" --global-option="--fcompiler=intelem"

During the search I found some references that this only works for setup.py build backends but with that new PEP517 (or so) it might need something different. No idea there.

migueldiascosta commented 6 months ago

During the search I found some references that this only works for setup.py build backends but with that new PEP517 (or so) it might need something different. No idea there.

Yeah, when building numpy 1.25.1, I get WARNING: Ignoring --global-option when building numpy using PEP 517.

I read somewhere that --config-settings is an alternative, but there seems to be some confusion about the syntax of the options passed there and I also couldn't get it to work. Writing to setup.cfg does work.

Flamefire commented 6 months ago

Yeah, when building numpy 1.25.1, I get WARNING: Ignoring --global-option when building numpy using PEP 517.

I asked about how to properly set the Fortran compiler for numpy and the answer was:

Numpy itself doesn't use a fortran compiler at build time

Are we doing something very superflous here and the numpy easyblock could just be (or derived from) PythonPackage? Spack only uses the config_fc for SciPy

Writing to setup.cfg does work.

What do you mean by "does work"? I tried to purposely break the "correct" build by exporting FC & F90 set to some bogus values and added a bogus value for config_fc in setup.cfg. The build did start at least (but failed to compile some C++ stuff, likely some patches missing in my numpy git checkout)
I would expect this to fail if the Fortran compiler was actually used.

migueldiascosta commented 6 months ago

I'm using the fujitsu compiler on a64fx. Without setting fcompiler to fujitsu, building numpy fails almost immediately when it tries to determine the version of the Fortran compiler with numpy/distutils/fcompiler/gnu.py instead of numpy/distutils/fcompiler/fujitsu.py

(i.e., it does try to use the fujitsu compiler because the environment variable points to it, but it fails simply because it uses the wrong command line option to get the version)

Since numpy.distutils is deprecated and will be removed, I suppose these checks could be superfluous, but they are still being executed...

Flamefire commented 6 months ago

I don't think it reuses anything,

I just checked the logs and it seems it does:

From what I can tell --fcompiler is something that is only applicable to numpy & packages using the numpy distutils like scipy. This strengthens my view that our EasyBlocks should directly derive from PythonPackage.

Without setting fcompiler to fujitsu, building numpy fails almost immediately when it tries to determine the version of the Fortran compiler with numpy/distutils/fcompiler/gnu.py instead of numpy/distutils/fcompiler/fujitsu.py

I found this https://github.com/numpy/numpy/blob/aad6c9cd1958309110f9892d10b28b1beea9c670/numpy/distutils/fcompiler/gnu.py#L74 which is where it fails for you isn't it?

The pip installation doesn't use this though. Hence I think this is not an issue. If I understood you correctly you already tried to build with pip on that a64fx system. Does the plain install (python -m pip install --prefix=/dev/shm/install --no-deps --ignore-installed --no-index --no-build-isolation .) without changing the setup.cfg work there?

migueldiascosta commented 6 months ago

The pip installation doesn't use this though.

I suppose it shouldn't, and I guess at some point it won't, but it still (as of 1.25.1 at least, haven't tried with 1.26 yet) does...

If I understood you correctly you already tried to build with pip on that a64fx system.

with older versions of numpy (e.g. 1.20) setting fcompiler via --global-option for pip (e.g. https://github.com/easybuilders/easybuild-easyblocks/pull/2434/files) works, but it doesn't on more recent versions, the --global-option is ignored

Does the plain install (python -m pip install --prefix=/dev/shm/install --no-deps --ignore-installed --no-index --no-build-isolation .) without changing the setup.cfg work there?

at least on 1.25.1 (from Scipy-bundle 2023.07), no, it fails on that version check with gnu.py instead of fujitsu.py.

To be clear, I'm not asking for anything here, I have a customized fortranpythonpackage.py that works (by changing setup.cfg in preparation for the test step with pip), these are simply my observations

Flamefire commented 6 months ago

I suppose it shouldn't, and I guess at some point it won't, but it still (as of 1.25.1 at least, haven't tried with 1.26 yet) does...

I tested 1.26 and the code isn't called using pip for me (I changed it to always raise an error), so this seems to have changed. Just wanted a confirmation of that.

with older versions of numpy (e.g. 1.20) setting fcompiler via --global-option for pip (e.g. https://github.com/easybuilders/easybuild-easyblocks/pull/2434/files) works, but it doesn't on more recent versions, the --global-option is ignored

Interesting PR. I guess it is WIP because of this issue? And it looks like it really was the intention to always use setup.py for building and pip for installation...
Is it possible that not the numpy version matters but the pip version? If you tested with different toolchains then pip would change with numpy and this change seems to affect pip itself.

at least on 1.25.1 (from Scipy-bundle 2023.07), no, it fails on that version check with gnu.py instead of fujitsu.py.

Do you have a backtrace of that? I created a PR for upstream but it likely won't get accepted. But we could use it as a patch for affected numpy versions

I have a customized fortranpythonpackage.py that works by changing setup.cfg in preparation for the test step with pip

By using the spack method?

To be clear, I'm not asking for anything here

I'm just trying to understand the problem and how to solve it. E.g. I'm surprised that Spack only needs this for scipy not numpy. And partially why the extra args for pip are required only for Fujitsu but not e.g. Intel

migueldiascosta commented 6 months ago

let me answer first the questions that don't require (re)running anything, I'll answer the ones that do when I get a chance

Interesting PR. I guess it is WIP because of this issue?

long story :) (I'm basically coming back to this now, after 3 years: https://github.com/easybuilders/easybuild/issues/704)

And it looks like it really was the intention to always use setup.py for building and pip for installation...

I'm fine with clocking the build time in the build step when both methods work, and indeed I didn't stop doing that, but it did bother me when the build with setup.py succeeded and then installation with pip failed, at identifying a compiler (that's why I said "I don't think it reuses anything"; I didn't look at the time it took when it did run, you're right, the install with pip is faster after the build with setup.py)

like you said originally, it's odd at least

Is it possible that not the numpy version matters but the pip version?

I suppose so, I've been sticking with the combinations in our common toolchains

I created a PR for upstream

I think the error I get is even before that, as soon as frt (the fujitsu fortran compiler) gets called with -dumpversion instead of -version it errors out, but I'll re-run to send you the backtrace (I do remember seeing an old_setup from numpy.distutils.core in the backtrace, so there may be indeed be different code paths depending on the different versions of pip, setuptools, etc.)

By using the spack method?

Yes, setting fcompiler = fujitsu in the [config_fc] section of setup.cfg achieves the same result as passing it as a --global-option to pip used to achieve

I'm surprised that Spack only needs this for scipy not numpy.

Yeah, it could be that I'm hitting a corner case, I'll try some different things

And partially why the extra args for pip are required only for Fujitsu but not e.g. Intel

I see now that we do have SciPy-bundle-2023.07-iimkl-2023a.eb so that didn't fail. I guess it could be that numpy is somehow auto-detecting the Intel compiler during the install step with pip...

Flamefire commented 6 months ago

I see now that we do have SciPy-bundle-2023.07-iimkl-2023a.eb so that didn't fail. I guess it could be that numpy is somehow auto-detecting the Intel compiler during the install step with pip...

Or maybe intels version interface is compatible enough with GCC. Given that is a fairly new numpy it is also possible pip install doesn't go through this code at all but just builds it with Meson (which is what they switched too, at least for pip/PEP517)

migueldiascosta commented 6 months ago

Given that is a fairly new numpy it is also possible pip install doesn't go through this code at all but just builds it with Meson

I had mentioned SciPy-bundle-2023.07 specifically because that's where I ran into the issues with the Fujitsu compiler, same versions of everything.

Ran some tests building SciPy-bundle-2023.07-iimkl-2023a.eb (numpy 1.25.1) the install with pip does still import numpy/distutils/fcompiler/gnu.py (it breaks if I add a patch with a syntax error), but it doesn't seem to run (it doesn't break if I patch it to always raise an error) the gnu_version_match that creates the problem with the Fujitsu compiler (indeed, you're right, that's exactly where the problem is), it gets the version with numpy/distutils/fcompiler/intel.py even without explicitly setting fcompiler

I created a PR for upstream but it likely won't get accepted. But we could use it as a patch for affected numpy versions

yes, your patch does allow installing numpy 1.25.1 with pip, using the Fujitsu compiler, without changing setup.cfg

I tested 1.26 and the code isn't called using pip for me (I changed it to always raise an error), so this seems to have changed. Just wanted a confirmation of that.

indeed, tested with SciPy-bundle-2023.11 (numpy 1.26.2), that numpy/distutils code is no longer called with pip

but it is still called via setup.py, and we're still pre-building with setup.py, so for instance in SciPy-bundle-2023.11-gfbf-2023b.eb we're using two completely different methods in the build and test/install steps?

I suppose for numpy >= 1.26 we should have already added something similar to https://github.com/easybuilders/easybuild-easyblocks/blob/743662d06caf460a9c3edd5e433ee37d5283f0a3/easybuild/easyblocks/s/scipy.py#L156-L161 to the numpy easyblock (?)

Flamefire commented 5 months ago

yes, your patch does allow installing numpy 1.25.1 with pip, using the Fujitsu compiler, without changing setup.cfg

Added to relevant ECs: https://github.com/easybuilders/easybuild-easyconfigs/pull/20817

indeed, tested with SciPy-bundle-2023.11 (numpy 1.26.2), that numpy/distutils code is no longer called with pip

but it is still called via setup.py, and we're still pre-building with setup.py, so for instance in SciPy-bundle-2023.11-gfbf-2023b.eb we're using two completely different methods in the build and test/install steps?

Does that mean SciPy-bundle-2023.11 without any changes for Fujitsu fails to build? That would still require the patch to be added.
And yes and no the methods are different but it is likely pip reuses the results from setup.py.

I suppose for numpy >= 1.26 we should have already added something similar to [scipy] to the numpy easyblock (?)

Looks like that would be possible. However the numpy docs state to use pip with a bit involved options for BLAS etc. passed via -C, like python -m pip install . -C-Dblas-order=openblas,mkl,blis -C-Dlapack-order=openblas,mkl,lapack

However the official procedure is the same for scipy: https://docs.scipy.org/doc/scipy/building/index.html#building-scipy-from-source. So to me it looks like we need an NumPySciPy Easyblock or similar.