Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problems with Python, improve installation, and improve tests #466

Merged
merged 52 commits into from
Feb 22, 2024

Conversation

rafaelab
Copy link
Member

@rafaelab rafaelab commented Feb 4, 2024

This PR supersedes #457.
It mainly improves the handling of python, but it also contains many other improvements/fixes:

  • Use CMake's built-in Python finder and remove CRPropa's Python finder.
  • Remove all references to Python2 and completely drops support
  • Fix a bug common to several tests when using clang (gtest cannot correctly trace exceptions leading to failures when calling EXPECT_THROW)
  • Fix a bug in testInteraction related to EMPairProduction, in OS X.
  • Possible solution the long-standing issue related to the magnetic lenses and python
  • Minor improvements in workflow
  • Apply some code style convention to *.i files and CMake
  • Improve code style in tests

One of the OSX tests (testPythonExtension.py) is expected to fail because of a problem applying NumPy functions to Vector3d in one of the tests (testArrayInterface). This does not happen, for example, in Ubuntu.
Even if this particular test fails, at least it is known and doesn't seem to affect CRPropa in any significant way.

@rafaelab
Copy link
Member Author

rafaelab commented Feb 9, 2024

Thanks for the review, @JulienDoerner
I think I've addressed most of the issues you've raised so far and after a long day of attempts I managed to get crpropa-example-test to run, to some extent.

I tried to make the installation default to Python_SITE_PACKAGES (actually, Python_SITELIB) only if the user is not trying to do a local installation using CMAKE_INSTALL_PREFIX. This way, we don't affect the previous behaviour too much, and people like myself who prefer to choose the installation paths are satisfied. What do you think?

I've made one further change: I commented out the test that was leading to the problem in the OSX tests. Unless you have any objections, I'd suggest we keep it that way until we figure out how to solve the problem. And it is better not to delete the corresponding lines yet, since they might be useful in the future.

@lukasmerten
Copy link
Member

lukasmerten commented Feb 12, 2024

It's more or less working for me.

However, I would also prefer an option to specify the CMAKE_INSTALL_PREFIX and Python_INSTALL_PACKAGE_DIR independently of each other. I need to set the CMAKE_INSTALL_PREFIX to something, which is different from the default /usr/local as we do not have write access to that location on our university machines. This leads in the current PR to installing the python module in CMAKE_INSTALL_PREFIX/crpropa which would require manually adapting the python PATH for my virtual envs...

EDIT:
The tests, however, are all passing for me. So the lensing bug seems to be fixed with this PR.

@JulienDoerner
Copy link
Member

Hey @rafaelab,

I think adding this variable Python_INSTALL_PACKAGE_DIR is a good idea. But for me I could not change (or see) it using ccmake. It only appers in the printed log.

And also this does not reproduce the old installation behaviour. In this case (given PREFIX) all installation parts are stored on the same level (python package PREFIX/crpropa, shared data PREFIX/share/crpropa, lib PREFIX/lib/libcrpropa.so and include PREFIX\include\CRPropa.h).
I would think the best way would be to allow for the user defined path but default it to PREFIX/lib/python3.X/crpropa. In this case the installation procedure would stay the same as in our installation documentation.

@rafaelab
Copy link
Member Author

Thanks for the feedback, @JulienDoerner and @lukasmerten.
Now the flag Python_INSTALL_PACKAGE_DIR is completely independent from CMAKE_INSTALL_PREFIX.

The remaining issue is what @JulienDoerner said about $PREFIX/lib/python3.X/crpropa. This is the internal structure of python packages. But in this case, the user explicitly wants to control all paths, so I fail to see a reason to keep it this way. Why add so many unnecessary layers when we can just have $Python_INSTALL_PACKAGE_DIR directly (at the level of $PREFIX or perhaps $Python_INSTALL_PACKAGE_DIR/python?

@JulienDoerner
Copy link
Member

JulienDoerner commented Feb 22, 2024

hey @rafaelab,

I think the change is good, as it is. My point for the python3.X/site-packages/ was only about the default value for the $Python_INSTALL_PACKAGE_DIR. But I agree that installing it directly in this level is the best solution.
And defaulting it into the Python_SITELIB will keep the previous installation behaviour.

@lukasmerten
Copy link
Member

Thanks for your effort @rafaelab.
Seems to work for me and I will approve this later.

Just for my records:
You can set the flag for the python installation path with
($~/build) cmake -DPython_INSTALL_PACKAGE_DIR=..

Just for curiosity:
Is there a way to add Python_INSTALL_PACKAGE_DIR to the parameters that are listed when I run ccmake?

@rafaelab
Copy link
Member Author

Just for my records: You can set the flag for the python installation path with ($~/build) cmake -DPython_INSTALL_PACKAGE_DIR=..

Just for curiosity: Is there a way to add Python_INSTALL_PACKAGE_DIR to the parameters that are listed when I run ccmake?

No idea, @lukasmerten. I did add a status message with the value of this flag, but I haven't invested too much time in adding it to the parameter. I thought it would show up there automatically.
In any case, I will keep this on my to-do list for next time I make general changes to CRPropa.

PyArrayObject *arr = NULL;
PyArray_Descr *dtype = NULL;
int ndim = 0;
npy_intp dims[NPY_MAXDIMS];
if (PyArray_GetArrayParamsFromObject(input, NULL, 1, &dtype, &ndim, dims, &arr, NULL) < 0)
{
if (PyArray_GetArrayParamsFromObject(input, NULL, 1, &dtype, &ndim, dims, &arr, NULL) < 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something goes wrong on this line. See issue #448 . Please, check behavior of this code on NumPy version 1.21.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants