qpv-research-group / solcore5

A multi-scale, python-based library for the modelling of solar cells and semiconductor materials
https://www.solcore.solar/
Other
133 stars 77 forks source link

`numpy.distutils.setup` deprecated and will be removed #238

Closed Abelarm closed 1 year ago

Abelarm commented 2 years ago

While I was working on other things I remembered that the setup of solcore utilise the numpy implementation, so I dig a bit and discovered that the setup implementation of numpy is going to be discontinued.

numpy.distutils is deprecated, and will be removed for Python >= 3.12.

LINK at doc

in their page about the migration they suggest to use setuptools which has the support to build Fortran.

Are they any special reason we you went with the setup from numpy?

This could be also a nice point to kickstart a modernisation of the setup process and files. I am not a big expert of this but I think can be useful to improve this a bit, what do you think?

I can take this task which should divided in several sub-task

dalonsoa commented 2 years ago

I was aware of numpy.distutils being discontinued, but forgot to add an issue about it. I was considering on using meson as a replacement - it is what Scipy uses, for example. But if setuptools now has support for fortran extensions - it hadn't back in the day, so we had to use numpy.distutils and f2py under the hood -, that's great! Possibly much easier to adapt the existing toolchain than implementing meson.

If you want to give this a try, absolutely go ahead! Please, let me know if you need anything from me.

eli-schwartz commented 2 years ago

Historically, setuptools didn't support fortran. The question is if "now" the status of that has changed.

in their page about the migration they suggest to use setuptools which has the support to build Fortran.

This page specifically says that it does NOT have support to build fortran.

Also, numpy does NOT suggest to use setuptools :) what they actually say is:

For projects that only use numpy.distutils for historical reasons, and do not actually use features beyond those that setuptools also supports, moving to setuptools is likely the solution which costs the least effort. To assess that, there are the numpy.distutils features that are not present in setuptools:

Indeed, doing something with the least effort is a big advantage -- but you are actually using numpy.distutils features so that is probably not going to work. :(

Abelarm commented 2 years ago

BIG SORRY!

I read that it has support Fortran, fast readings lead to mistakes :( Than the task is not so simple and a better plan is needed

eli-schwartz commented 2 years ago

If that plan is "follow numpy and scipy and use Meson" then I will be happy to offer guidance if needed, BTW. :)

dalonsoa commented 2 years ago

I think that should be the plan - it is what all the big ones are doing. I'm trying to make time to refactor some parts of the code to make it clearer for the end users and modular, so I cannot take on this, but if @Abelarm is still keen on giving this a try, that would be great!

Abelarm commented 1 year ago

Hi guys back with some news.

First of all since I never used meson I tried the tutorial from numpy.f2py using-via-meson. Which works fine.

Then I tried to modify their build to match our needs.

I created the meson.build (draft version) inside the root with:

project('solcore',
    'c',
    version: '0.1',
    default_options : [
        'warning_level=0',
    ]
)

add_languages('fortran', 'cython')

py_mod = import('python')
py3 = py_mod.find_installation('python3')
py3_dep = py3.dependency()
message(py3.path())
message(py3.get_install_dir())

install_subdir(
  'solcore',
  install_dir: py3.get_install_dir(pure: false)
)

subdir('solcore/poisson_drift_diffusion')

and another one inside solcore/poisson_drift_diffusion This one is responsible of building the fortran file. Needed to be modified from the tutorial because we cannot use anymore full paths

incdir_numpy = run_command(py3,
  ['-c', 'import os; import numpy; cwd = os.getcwd(); os.chdir(".."); numpy_path = numpy.get_include(); print(os.path.relpath(numpy_path, cwd));'],
  check : true
).stdout().strip()

incdir_f2py = run_command(py3,
    ['-c', 'import os; import numpy.f2py; cwd = os.getcwd(); os.chdir(".."); f2py_path = numpy.f2py.get_include(); print(os.path.relpath(f2py_path, cwd))'],
    check : true
).stdout().strip()

message(incdir_numpy)
message(incdir_f2py)

ddModule_source = custom_target('ddModelmodule.c',
                            input : ['DDmodel-current.f95'],
                            output : ['ddModelmodule.c', 'ddModel-f2pywrappers2.f90'],
                            command : [py3, '-m', 'numpy.f2py', '-m', 'ddModel', '@INPUT@', '--lower',
                                       '--build-dir', 'solcore/poisson_drift_diffusion']
                            )

inc_np = include_directories(incdir_numpy, incdir_f2py)

py3.extension_module('ddModel',
                     'DDmodel-current.f95',
                     ddModule_source,
                     incdir_f2py / 'fortranobject.c',
                     include_directories: inc_np,
                     dependencies : py3_dep,
                     install_dir: py3.get_install_dir(pure: false) / 'solcore' / 'poisson_drift_diffusion',
                     install : true)

It seems that it works but for some test I get: AttributeError: module 'solcore.poisson_drift_diffusion' has no attribute 'iv_pdd'

now I don't know if the error is in the build or in the python code, resume of the pytest:

image

Now I think we have more test that use the ddModel no? so I am confused if the build is correct or there is something missing...

link at branch

:rocket:

eli-schwartz commented 1 year ago

Are you testing the installed version or trying to run pytest with the source tree? Since Meson doesn't support in-tree builds you have to use meson install first.

(The meson-python build backend has an open ticket for adding editable installs support.)

SciPy uses a dev.py script to conveniently wrap this, and https://github.com/scientific-python/devpy tries to make this generically useful across any project.

Abelarm commented 1 year ago

Are you testing the installed version or trying to run pytest with the source tree? Since Meson doesn't support in-tree builds you have to use meson install first.

(The meson-python build backend has an open ticket for adding editable installs support.)

SciPy uses a dev.py script to conveniently wrap this, and https://github.com/scientific-python/devpy tries to make this generically useful across any project.

I did pip install ".[test]" to install the package locally and then pytest to run the test

dalonsoa commented 1 year ago

Yes, there are more tests using the PDD solver (you can see in your error message that the second line is also about a PDD-related test failing. And some examples also use the PDD. I don't know why all those tests related to optics are failing - they have nothing to do with the PDD.

As @eli-schwartz mentions, I suspect you might be testing the source tree version of Solcore rather than the one you installed with pip install ".[test]". This only adds to the installation process the dependencies related to testing - pytest and the sort - but the tests themselves still live in the source tree. The only way I can think of making sure that the source tree version of solcore is not used is to rename the solcore folder as solcore_source, just for the time being, so when running pytest, the test don't find any solcore in the current directory and checks the installed one.

Not checked this myself, but I think it should work.

eli-schwartz commented 1 year ago

It may also be possible to change how pytest is getting run -- e.g. python -m pytest adds the current working directory to PYTHONPATH before importing and running the pytest module, while pytest simply runs pytest directly -- or pass options to pytest itself to change how it tries to inject the test runner into PYTHONPATH (--import-mode=append).

I would tend to prefer that over moving source files around as that may mess with the rebuild detection.

Abelarm commented 1 year ago

Hi guys I am back at it! Sorry took me so much time...

First of all I noticed that when I do meson setup build with this meson:

project('solcore',
    'c',
    version: '0.1',
    meson_version: '>= 0.64.0',
    default_options : [
        'buildtype=debugoptimized',
        'c_std=c99',
        'cpp_std=c++14',
    ]
)

add_languages('fortran', 'cython')

py_mod = import('python')
py = py_mod.find_installation('python3', pure: false)
py_dep = py.dependency()
message(py.full_path())
message(py.get_install_dir())

subdir('solcore')
install_subdir('tests', install_dir: py.get_install_dir() / 'solcore/tests')

I go from the message:

Program python3 found: YES (/home/luigi/Workspace/solcore5/venv/bin/python3)
Found pkg-config: /usr/bin/pkg-config (0.29.2)
Message: /home/luigi/Workspace/solcore5/venv/bin/python3
Message: /usr/local/lib/python3/dist-packages/  < ---- WRONG ONE?
Build targets in project: 2

why is /usr/local/lib/python3/dist-packages/??

Infact when I install it with meson install --only-changed -C build --destdir build-install it installs inside the: image

I also tried with devpy (which I think is pretty usefull) but I have a problem that devpy uses the system meson and not the one inside virtualenv

Can you guys help me?

eli-schwartz commented 1 year ago

Try configuring with meson setup build/ -Dpython.install_env=venv (either venv or auto should work).

See https://mesonbuild.com/Builtin-options.html#python-module for more details.

Abelarm commented 1 year ago

Thank you @eli-schwartz I am learning a lot!

So back on the topic, with the command meson setup build/ -Dpython.install_env=venv now the messages are correct:

Message: /home/luigi/Workspace/solcore5/venv/bin/python3
Message: /home/luigi/Workspace/solcore5/venv/lib/python3.10/site-packages/
Message: ../../venv/lib/python3.10/site-packages/numpy/core/include
Message: ../../venv/lib/python3.10/site-packages/numpy/f2py/src
Build targets in project: 2

and after installing with meson install --only-changed -C build --destdir build-install I have: image

I just for the sake of testing I copied the test inside the package...

So until here everything seems to be ok!

so now cd inside that directory

... build-install/home/luigi/Workspace/solcore5/venv/lib/python3.10/site-packages/ and run pytest

now I get:

image

all 17 errors are: ImportError: cannot import name 'calculate_rat_rcwa' from 'solcore.optics.rcwa'

could it be related to fact that I do not have the S4 installed? @dalonsoa because some of them are:

image

but some of them are:

image

I think we are really close to a first draft of moving to meson :rocket:

dalonsoa commented 1 year ago

This looks like a great progress!

@Abelarm , that seems to be the case. In the CI, S4 is installed in Ubuntu, so tests run fine. But in truth, the tests should be skip if there is no S4 installation, as it is an optional feature, regardless of the platform.

I'll open an issue now to tackle that as soon as possible. In the meantime, if you can move forward knowing that those tests are failing because a known, totally unrelated, issue, that would be great.

Abelarm commented 1 year ago

We did it!! :rocket: :tada:

Look at this action run build with meson using the command python -m devpy build and test run with python -m devpy test

thank you very much to @eli-schwartz now I will clean up a bit several things:

What else do you think is necessary?

Abelarm commented 1 year ago

Ok back with some other news: LINK AT ACTION

Abelarm commented 1 year ago

Great news!! :rocket:

we now have:


TODO:

eli-schwartz commented 1 year ago

@Abelarm

devpy lists python 3.7 support, so it should actually support it and failing to support it is a bug. There are actually two issues. I've fixed both of them and added 3.7 to devpy's CI.

Abelarm commented 1 year ago

:tada: Happy new year :tada:

With the 2023 we have an update!

One question for @eli-schwartz why in the hell for having gfortran working in windows this strange command is needed? image I copied from the scipy action... it made me loose several hours :disappointed:

eli-schwartz commented 1 year ago

SciPy uses rtools as a conveniently packaged modern mingw-w64 toolchain for Windows. It's probably not the only option, but it does seem to work well for them. :)

You do need to do something, and MSVC is not suitable for fortran because it's only got a C/C++ compiler, and getting the right setup can be more of an art than a science at times. So once you hit on a good combination, even if it's provided by the R community rather than the python community, people tend to settle on that and stick with it I guess.

Abelarm commented 1 year ago

SciPy uses rtools as a conveniently packaged modern mingw-w64 toolchain for Windows. It's probably not the only option, but it does seem to work well for them. :)

You do need to do something, and MSVC is not suitable for fortran because it's only got a C/C++ compiler, and getting the right setup can be more of an art than a science at times. So once you hit on a good combination, even if it's provided by the R community rather than the python community, people tend to settle on that and stick with it I guess.

Yeah I got it, but it's strange that the 'r-tools' is already installed in the windows image, so it kind of silent override the installation... Anyway not complaining if it works 🤷🏽‍♂️

Abelarm commented 1 year ago

:tada: HABEMUS WHEELS :tada:

I have completed the action for the creation of the wheels LINK

Since I needed to refactor the build I moved to cibuildwheel which gives more flexibility and it's a single step for the the three os.

I have already tested the Linux one and it works fine. Could you guys help me out test the os that are missing @dalonsoa @phoebe-p (the MacOs is both x86_64 and arm64) because I do not have them installed? (the .whl are inside the above link)

To test:

EDIT: There is one small problem to figure it out with the build of windows does not include the README.md due to some char issues. -> the new release of pyproject-metadata should fix it. SOLVED :hammer:

Abelarm commented 1 year ago

Separate question regarding the cross compilation on cibuildwheel for @eli-schwartz.

I am trying to cross compile the arm64 from the x86_64 now I got stuck to the delocate part of the build:

delocate.delocating.DelocationError: Some missing architectures in wheel
Required arch arm64 missing from /usr/local/Cellar/gcc/12.2.0/lib/gcc/current/libgfortran.5.dylib
Required arch arm64 missing from solcore/poisson_drift_diffusion/ddModel.cpython-38-darwin.so

I have both the gfortran for x86 and the one for arm but I am not able to set it up in meson so that the the host compiler is the arm one and the build is the x86 one. I have the following cross_file.ini

[binaries]
fortran = 'gfortran'
c = 'gcc'

[host_machine]
system = 'macos'
cpu_family = 'arm64'
cpu = 'arm64'
endian = 'little'

[build_machine]
system = 'macos'
cpu_family = 'x86_64'
cpu = 'x86_64'
endian = 'little'

is it correct/doable the way I am doing it?

eli-schwartz commented 1 year ago

Have you tried specifying the exact paths to the gfortran and GCC executables? You may be picking up the wrong ones -- it's generally preferable in cross compilation to be as exact as possible.

Abelarm commented 1 year ago

Have you tried specifying the exact paths to the gfortran and GCC executables? You may be picking up the wrong ones -- it's generally preferable in cross compilation to be as exact as possible.

That's exactly my problem:

  1. Should I specify one for each architecture? How?
  2. Is there a special GCC and gfortran thare used for cross compile? Where do I find them?
  3. Where I should specify this exact path? In meson.build or in the cross-file.ini?
Abelarm commented 1 year ago

so the thing I am trying to achieve is this:

I have the x86 version in /usr/local/bin/gfortran and I installed the arm version /usr/local/bin/gfortran_arm64

and then: image

but this is not supported or maybe I am doing it wrongly :( Is this what you meant?

eli-schwartz commented 1 year ago

/usr/local/bin/gfortran_arm64 isn't a programming language.

That's exactly my problem:

1. Should I specify one for each architecture? How?

You only need this for the architecture you are building (arm64). Do this via the cross-file.ini

2. Is there a special GCC and gfortran thare used for cross compile? Where do I find them?

Seems like the special gfortran used for cross-compile is /usr/local/bin/gfortran_arm64, right?

3. Where I should specify this exact path? In meson.build or in the cross-file.ini?

In the cross-file.ini -- Meson specifically separates this to keep the project build files "clean", if people want to build solcore5 on Windows or Linux or FreeBSD, they shouldn't need to patch the build files to specify the exact location of their compiler, all they should need to do is pass their own cross-file.ini that describes their build environment. (And that cross-file.ini can be shared for ANY projects that need to build fortran sources.)

Also, you don't need to add if/else soup to your meson.build to define each platform's compiler. :)

So, your cross-file.ini should look like this:


[binaries]
fortran = '/usr/local/bin/gfortran_arm64'
c = '/usr/local/binl/gcc_arm64'
Abelarm commented 1 year ago

Thank you very much for your explanation @eli-schwartz! :bowing_man:

But I guess I found out the problem, the /usr/local/bin/gfortran_arm64 is not a cross compiler x86_64->arm64 it is just a "normal" compiler for arm64 in fact when I try your suggestions I get:

meson.build:29:0: ERROR: Unknown compiler(s): [['/usr/local/bin/gfortran_arm64']]
The following exception(s) were encountered:
Running `/usr/local/bin/gfortran_arm64 --version` gave "[Errno 86] Bad CPU type in executable: '/usr/local/bin/gfortran_arm64'"
Running `/usr/local/bin/gfortran_arm64 -V` gave "[Errno 86] Bad CPU type in executable: '/usr/local/bin/gfortran_arm64'"

so unless I found the x86_64->arm64 cross compiler I cannot proceed, right?

Abelarm commented 1 year ago

:tada: I DID IT!!!! :tada:

We now have solcore for arm64 :apple:

I really want to thank for @eli-schwartz, without his knowledge and patience I would not have done it (not even the normal build on linux)

going to update the link to where to download the wheels so you guys can test it.

eli-schwartz commented 1 year ago

Fantastic! Happy to help.

phoebe-p commented 1 year ago

I have already tested the Linux one and it works fine. Could you guys help me out test the os that are missing @dalonsoa @phoebe-p (the MacOs is both x86_64 and arm64) because I do not have them installed? (the .whl are inside the above link)

I will test both the MacOS wheels (recently got an x86_64 Python environment running on my Mac as well using Rosetta).

Thanks all for your work on this!

Edit: I have tested both the x86 and arm64 versions and they work, including compiling the PDD 🎉. I only tested the Python 3.10 versions but can test the other ones if that is necessary

2nd edit: I have also tested the Windows wheel with Python 3.10 and it works.

Abelarm commented 1 year ago

YEAAAAH!! 🎉

I was a bit sceptical about the arm one... but it works!🚀

Thank you @phoebe-p, just one python is more than enough!

stefanv commented 1 year ago

@Abelarm

devpy lists python 3.7 support, so it should actually support it and failing to support it is a bug. There are actually two issues. I've fixed both of them and added 3.7 to devpy's CI.

Apologies for the delay, @eli-schwartz's PR to devpy has now been merged.

Abelarm commented 1 year ago

@stefanv no need to apologies, holidays are important!

Thank you for your work, I really like devpy 😍

phoebe-p commented 1 year ago

@all-contributors please add @eli-schwartz for infra

allcontributors[bot] commented 1 year ago

@phoebe-p

I've put up a pull request to add @eli-schwartz! :tada:

phoebe-p commented 1 year ago

Addressed by #242 and #245.