class_public icon indicating copy to clipboard operation
class_public copied to clipboard

Python package at pypi.org

Open uweschmitt opened this issue 2 years ago • 29 comments

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.

uweschmitt avatar Nov 15 '22 14:11 uweschmitt

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?

ntessore avatar Nov 16 '22 14:11 ntessore

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.

schoeneberg avatar Nov 16 '22 14:11 schoeneberg

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.

ntessore avatar Nov 16 '22 14:11 ntessore

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.

itrharrison avatar Nov 16 '22 17:11 itrharrison

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!

schoeneberg avatar Nov 17 '22 09:11 schoeneberg

Have added you as an owner 👍

itrharrison avatar Nov 17 '22 10:11 itrharrison

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!

schoeneberg avatar Nov 17 '22 11:11 schoeneberg

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

itrharrison avatar Nov 17 '22 11:11 itrharrison

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.

ntessore avatar Nov 17 '22 12:11 ntessore

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?

schoeneberg avatar Nov 17 '22 16:11 schoeneberg

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)

itrharrison avatar Nov 17 '22 17:11 itrharrison

@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.

uweschmitt avatar Nov 17 '22 17:11 uweschmitt

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.

ntessore avatar Nov 17 '22 17:11 ntessore

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!

schoeneberg avatar Nov 18 '22 08:11 schoeneberg

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 avatar Nov 18 '22 09:11 uweschmitt

@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

uweschmitt avatar Nov 18 '22 09:11 uweschmitt

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 avatar Nov 18 '22 09:11 itrharrison

@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.

itrharrison avatar Nov 25 '22 12:11 itrharrison

+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.

JCGoran avatar Nov 28 '22 13:11 JCGoran

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 avatar Nov 30 '22 08:11 snesseris

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

# import classy module
!pip install classy==2.9.4
from classy import Class

snesseris avatar Nov 30 '22 08:11 snesseris

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.

schoeneberg avatar Nov 30 '22 08:11 schoeneberg

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

ntessore avatar Nov 30 '22 09:11 ntessore

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+.

JCGoran avatar Nov 30 '22 10:11 JCGoran

@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.

snesseris avatar Nov 30 '22 16:11 snesseris

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.

schoeneberg avatar Dec 01 '22 09:12 schoeneberg

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).

JCGoran avatar Dec 01 '22 10:12 JCGoran

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.

schoeneberg avatar Dec 01 '22 17:12 schoeneberg

The update works great, thanks!

snesseris avatar Dec 01 '22 19:12 snesseris