lesgourg / class_public

Public repository of the Cosmic Linear Anisotropy Solving System (master for the most recent version of the standard code; GW_CLASS to include Cosmic Gravitational Wave Background anisotropies; classnet branch for acceleration with neutral networks; ExoCLASS branch for exotic energy injection; class_matter branch for FFTlog)
220 stars 291 forks source link

Python package at pypi.org #502

Open uweschmitt opened 1 year ago

uweschmitt commented 1 year ago

The package at https://pypi.org/project/classy/ is outdated. In case you are in charge of this repository: can you please update or disable the repo? Constantly causes confusions and version issues here.

ntessore commented 1 year ago

Updating the repository will probably take a fair amount of work. I am happy to delete the project on PyPI (or yank the releases) if the CLASS maintainers wish (in fact, I have offered in #371 to transfer the project to them, no reply) but since the published releases are clearly marked with the correct versions, and continue to work fine, why would that be necessary?

schoeneberg commented 1 year ago

Dear @ntessore , this is something I would be happy to dig into. Having a pip-installable CLASS might be nice. Would you be happy to give me access/ownership of the PyPI project? I will also try to see what @lesgourg thinks later.

Edit: Just for your information, I am Dr. Nils Schöneberg, and was a PhD student of Julien in the past, and am currently actively developing class.

ntessore commented 1 year ago

Hi @schoeneberg, no problem, I only need your PyPI username. I think the plan was always to remove me and @itrharrison once we handed the project over and everything is running, so feel free to do that.

NB: In case you do want to get rid of the project, no objections from me, but just FYI there are always about 100 or so downloads per month, so it may cause some disruption.

itrharrison commented 1 year ago

I'm also happy to help and / or be removed. But I will add my voice to that saying it would be a shame to delete the project and not replace it.

schoeneberg commented 1 year ago

Dear @itrharrison , thanks a lot. I am not planning on removing the project. Instead, I would be planning to update it to the newest CLASS version. I created an account on PyPI with the username "nilsor" (https://pypi.org/user/nilsor/). Let me know if you can find me!

itrharrison commented 1 year ago

Have added you as an owner 👍

schoeneberg commented 1 year ago

Dear @itrharrison . Thanks a lot. I already built the files necessary, but need to upload to test.pypi.org to test these first. I assume there you have already also created the classy project? Could you add me to the TestPYpi as well? Thanks!

itrharrison commented 1 year ago

It seems only @ntessore is an owner there https://test.pypi.org/project/classy/ 🤔 🙈 .

ntessore commented 1 year ago

I have added https://test.pypi.org/user/nilsor/ as an owner there. Sorry about that, I did not even remember using the test index.

schoeneberg commented 1 year ago

Dear @ntessore : Thanks a lot for adding me! For both of @ntessore and @itrharrison (and @uweschmitt) : I just uploaded the class 3.2.0. After some trials and tribulations over on test.pypi.org, I managed to find something that works for me. Can you please check if pip install classy works for you and correctly installs the newest class version?

itrharrison commented 1 year ago

In a clean conda env (after installing numpy and cython) I can successfully pip install classy==2.9.4 and also use it (@ntessore 's highest version). But pip install classy installs version 3.2.0 successfully but then errors on import:

>>> import classy
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: dlopen(/Users/ianharrison/opt/anaconda3/envs/classy-pip/lib/python3.11/site-packages/classy.cpython-311-darwin.so, 0x0002): symbol not found in flat namespace (_background_at_tau)
uweschmitt commented 1 year ago

@itrharrison this error was also report.ed at https://github.com/lesgourg/class_public/issues/384 and https://github.com/lesgourg/class_public/issues/416 and might be related to M1 Macs.

More work but the proper solution would be to upload pre-compiled wheel packages. I got classy compiled locally on my M1-Mac including OpenMP by using an ARM enabled gcc-12 from homebrew. pip install from a source package would not be able to consider this special setup easily.

ntessore commented 1 year ago

You probably don’t want to compile any wheel yourself, and instead use cibuildwheel to upload wheels for all platforms automatically.

If you are not familiar, it’s easy to set up, but beware that it eats through CI minutes. So you probably only want to build on releases or manually. You probably also want to skip all pypy builds, given how rare it is in our community.

schoeneberg commented 1 year ago

Dear @uweschmitt , @itrharrison , @ntessore : Seems to me like this is related to the existing installation issues on Mac with OpenMP then. The "optimal" way to solve this issue then would be to make the makefile (or at least the setup.py that controls the automatic pip installation) responsive to the underlying OS, setting the correct installation flags during compilation. However, since I personally don't own a mac, that's something I cannot easily debug. If you're happy if you can see whether you can make the python3 setup.py build_ext --inplace work directly in the unzipped tar file from here https://pypi.org/manage/project/classy/release/3.2.0/, that would be very kind. In any case, thanks for testing around with the pip installable classy!

uweschmitt commented 1 year ago

Dear @schoeneberg the problem no Mac cannot be solved with improving the Makefile alone, the default cc on Mac has no OpenMP support and to enable OpenMP you also have to install a proper gcc using homebrew.

uweschmitt commented 1 year ago

@schoeneberg you provided a link which requires password auth, the public link to the source is https://files.pythonhosted.org/packages/ba/e3/a35b0b35f6e5d30ed6df6a11132caed47513e5c8842b434bff8b4a548c32/classy-3.2.0.tar.gz

itrharrison commented 1 year ago

Thanks both. I should point out that my problem above was on an intel mac, not an M1 one. I will try and find time to have a more concerted go at trying to get it to build.

itrharrison commented 1 year ago

@uweschmitt @schoeneberg would it be possible for the pip installable version to be without openmp? My (and I believe a few others') use case for the pip package is for use in github CI environments to run tests where openmp is probably not necessary but reliable installation is.

JCGoran commented 1 year ago

+1, a PyPI package would be much appreciated! If not possible, a Conda package (see #491) would also work since one can bundle everything but the kitchen sink when making one of those.

snesseris commented 1 year ago

Hi all,

Thanks for making a pypi package! Using it with Google colab I get the following error with the latest version though:

CosmoComputationError: 

Error in Class: thermodynamics_init(L:342) :error in thermodynamics_helium_from_bbn(ppr,pba,pth);
=>thermodynamics_helium_from_bbn(L:545) :could not open fA with name /home/nilsor/codes/class_public/class_public/external/bbn/sBBN_2017.dat and mode "r"

as it seems the path sBBN_2017.dat is now hardcoded in the wheel!?

snesseris commented 1 year ago

Ok, as a workaround installing a previous version works for now:

# import classy module
!pip install classy==2.9.4
from classy import Class
schoeneberg commented 1 year ago

Hi all,

Thanks for making a pypi package! Using it with Google colab I get the following error with the latest version though:

CosmoComputationError: 

Error in Class: thermodynamics_init(L:342) :error in thermodynamics_helium_from_bbn(ppr,pba,pth);
=>thermodynamics_helium_from_bbn(L:545) :could not open fA with name /home/nilsor/codes/class_public/class_public/external/bbn/sBBN_2017.dat and mode "r"

as it seems the path sBBN_2017.dat is now hardcoded in the wheel!?

Dear @snesseris , thanks for reporting that bug. That's very unexpected and I am not yet sure what's the root cause for that. In principle that directly location should not be hard-coded. I think it might have happened when I did my first test, which automatically generated the wheel (I didn't build a wheel before). I will have to think about how to avoid this particular issue. Thanks for bringing it up.

As for @itrharrison 's suggestion of not using OpenMP, I am currently looking into using C++ threads for this, but C++ sadly comes with a lot of other unavoidable consequences (like the usual "malloc" reporting as problematic during compilation, or different handling of external functions and linking of code). I will continue seeing whenever I have a bit of free time.

ntessore commented 1 year ago

@schoeneberg In #371 I had to work around the hard-coded __CLASSDIR__, perhaps that is still the case?

JCGoran commented 1 year ago

Hi all,

Thanks for making a pypi package! Using it with Google colab I get the following error with the latest version though:

CosmoComputationError: 

Error in Class: thermodynamics_init(L:342) :error in thermodynamics_helium_from_bbn(ppr,pba,pth);
=>thermodynamics_helium_from_bbn(L:545) :could not open fA with name /home/nilsor/codes/class_public/class_public/external/bbn/sBBN_2017.dat and mode "r"

as it seems the path sBBN_2017.dat is now hardcoded in the wheel!?

I'm about 99% sure this is still caused by #255, as CLASS does not include the file at compile-time, bur rather loads it at run-time. I made a very lazy fix in https://github.com/JCGoran/class_public/commit/f129d890188c79949e7bf2edfe03dc0b1ba62cec, that nonetheless works as intended (it basically #includes the file directly in the source file at compile-time, so consequently libclass.a has it and the original CLASS dir can safely be moved/deleted), if someone wants to apply something similar to CLASS v3+.

snesseris commented 1 year ago

@JCGoran Indeed this might be a good solution in general, as I have also encountered the same error again eg when copy-pasting the whole CLASS directory to another location (eg to a cluster). Then one has to compile again as the path is hardcoded during compile-time.

schoeneberg commented 1 year ago

Dear @snesseris and @JCGoran , the solution made by @JCGoran is probably not something we want to implement in CLASS, since it goes against one of the core principles of CLASS (no hard-coding), and it disables the ability to change the BBN file using the sBBN file option. As such, the load at run-time is a desired behavior and will not be removed.

The real problem is the mismatch between the run-time loading of the file, and the compile-time definition of the default path. I realized that this can probably be fixed by putting a default path to NULL (instead of a compile-time string), and finding the path programatically once a NULL path is encountered (i.e. it's not overwritten within the input module). I will make a PR and push something that should work better. The new version will then hopefully be immune to these kinds of bugs.

JCGoran commented 1 year ago

Dear @snesseris and @JCGoran , the solution made by @JCGoran is probably not something we want to implement in CLASS, since it goes against one of the core principles of CLASS (no hard-coding), and it disables the ability to change the BBN file using the sBBN file option. As such, the load at run-time is a desired behavior and will not be removed.

The real problem is the mismatch between the run-time loading of the file, and the compile-time definition of the default path. I realized that this can probably be fixed by putting a default path to NULL (instead of a compile-time string), and finding the path programatically once a NULL path is encountered (i.e. it's not overwritten within the input module). I will make a PR and push something that should work better. The new version will then hopefully be immune to these kinds of bugs.

I mean, one could do both: just include the default file at compile-time, and allow the user specify a custom one at run-time. This would simultaneously get rid of the "I moved my CLASS directory and everything broke" syndrome, and would not be hard-coding the values (IMHO, I don't consider "setting a sane default" to be "hard-coding"). Of course, the other alternative would be to separate the library and the Python wrapper, and make a proper make install procedure (so it installs the C headers, any data, and the library separately in some standard location, allowing the wrapper to be linked in a later step), but that's probably a bit more involved to setup (though would undoubtedly make it easier to integrate CLASS in other software).

schoeneberg commented 1 year ago

Dear @snesseris and @JCGoran , I fixed the installation through a very tiny modification of the Makefile, as well as making the setup.py more complicated. I think it should work now on other systems as well. I still haven't fixed the OpenMP, though, that's still a longer-term issue.

snesseris commented 1 year ago

The update works great, thanks!