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

Making X-PSI support Cython3 #434

Merged
merged 18 commits into from
Dec 5, 2024
Merged

Making X-PSI support Cython3 #434

merged 18 commits into from
Dec 5, 2024

Conversation

thjsal
Copy link
Contributor

@thjsal thjsal commented Dec 2, 2024

Modifed X-PSI so that it can be installed with Cython3. I fixed the errors one by one as they appeared, based on suggested solutions., e.g. by adding "noexcept" before "nogil". For some reason I also needed to rename "I" with another name in some functions. Quite many warnings still left though. And X-PSI crashes now when importing it with the following error:

/=============================================\
| X-PSI: X-ray Pulse Simulation and Inference |
|---------------------------------------------|
|                Version: 2.2.7               |
|---------------------------------------------|
|      https://xpsi-group.github.io/xpsi      |
\=============================================/

Traceback (most recent call last):
  File "/home/saltuomo/xpsi/xpsi_group/xpsi/examples/examples_fast/Modules/main.py", line 6, in <module>
    import xpsi
  File "/home/saltuomo/.conda/envs/xpsi_py3_cy3/lib/python3.12/site-packages/xpsi-2.2.7-py3.12-linux-x86_64.egg/xpsi/__init__.py", line 63, in <module>
    from xpsi.tools import set_phase_interpolant, set_energy_interpolant
  File "xpsi/tools/__init__.pyx", line 10, in init xpsi.tools
  File "xpsi/tools/phase_integrator.pyx", line 1, in init xpsi.tools.phase_integrator
TypeError: C function xpsi.tools._get_phase_interpolant has wrong signature (expected gsl_interp_type *(void), got gsl_interp_type const *(void))

Syntaxes changes so that x-psi compiles without errors. Does still fail when importing x-psi.
@thjsal thjsal changed the title [WIP] Making XCython3 [WIP] Making X-PSI support Cython3 Dec 2, 2024
@thjsal
Copy link
Contributor Author

thjsal commented Dec 3, 2024

I fixed the error with the commit above (needed to just add word "const" also in the function definition in __init__.pxd file). Now I get this error when importing X-PSI:

/=============================================\
| X-PSI: X-ray Pulse Simulation and Inference |
|---------------------------------------------|
|                Version: 2.2.7               |
|---------------------------------------------|
|      https://xpsi-group.github.io/xpsi      |
\=============================================/

Traceback (most recent call last):
  File "/home/saltuomo/xpsi/xpsi_group/xpsi/examples/examples_fast/Modules/main.py", line 6, in <module>
    import xpsi
  File "/home/saltuomo/.conda/envs/xpsi_py3_cy3/lib/python3.12/site-packages/xpsi-2.2.7-py3.12-linux-x86_64.egg/xpsi/__init__.py", line 70, in <module>
    from xpsi.Likelihood import Likelihood
  File "/home/saltuomo/.conda/envs/xpsi_py3_cy3/lib/python3.12/site-packages/xpsi-2.2.7-py3.12-linux-x86_64.egg/xpsi/Likelihood.py", line 11, in <module>
    from xpsi.Star import Star
  File "/home/saltuomo/.conda/envs/xpsi_py3_cy3/lib/python3.12/site-packages/xpsi-2.2.7-py3.12-linux-x86_64.egg/xpsi/Star.py", line 5, in <module>
    from xpsi.Photosphere import Photosphere
  File "/home/saltuomo/.conda/envs/xpsi_py3_cy3/lib/python3.12/site-packages/xpsi-2.2.7-py3.12-linux-x86_64.egg/xpsi/Photosphere.py", line 8, in <module>
    from xpsi.HotRegion import HotRegion
  File "/home/saltuomo/.conda/envs/xpsi_py3_cy3/lib/python3.12/site-packages/xpsi-2.2.7-py3.12-linux-x86_64.egg/xpsi/HotRegion.py", line 3, in <module>
    from xpsi.cellmesh.mesh_tools import allocate_cells as _allocate_cells
  File "xpsi/cellmesh/mesh_tools.pyx", line 1, in init xpsi.cellmesh.mesh_tools
  File "xpsi/surface_radiation_field/__init__.pyx", line 1, in init xpsi.surface_radiation_field
ImportError: dynamic module does not define module export function (PyInit___init__)

@thjsal thjsal changed the title [WIP] Making X-PSI support Cython3 Making X-PSI support Cython3 Dec 5, 2024
@thjsal
Copy link
Contributor Author

thjsal commented Dec 5, 2024

So to summarize: The error mentioned in the previous comment was fixed (thanks to @evertrol) by changing the name of the cython file __init__.pyx to core.pyx, and creating a new python file __init__.py, in which the only content is:
from .core import *.

Also needed to change the setup.py file to include the core-file instead of the init-file.

I did this change both for the surface_radiation_field directory (from where the error was coming from), but also for the tools directory to be consistent (in the latter case I needed to change __init__.pxd to core.pxd and update some import paths in other files, which were importing functions from the init-file before).

The new code runs now without errors. Originally, I noticed that the new Cython3 code was consistently about 20% slower than the old Cython version when running the fast X-PSI example. However, this difference seems to have been vanished after I removed the remaining installation warnings by replacing all the nogil with noexcept nogil in the function definitions (so not only in the those places that were causing errors).

@MHoogkamer
Copy link
Contributor

So far, I checked this in a new environment with Cython 3. Installation works and the example cases in X-PSI as well. I also checked that this implementation works when installing it with Cython~0.29.

Currently, still checking this on Snellius

…this we need to switch to the 2024 stack, which we need to do anyways because the 2022 stack will be deprecated.
@MHoogkamer
Copy link
Contributor

Last commit also fixes #474 related to updating the Snellius instructions.

@MHoogkamer MHoogkamer closed this Dec 5, 2024
@MHoogkamer MHoogkamer reopened this Dec 5, 2024
@MHoogkamer
Copy link
Contributor

Accidentally closed this merge request instead of accepting it

Copy link
Contributor

@MHoogkamer MHoogkamer left a comment

Choose a reason for hiding this comment

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

Looks good Tuomo!

@DevarshiChoudhury DevarshiChoudhury merged commit ebfeb3c into main Dec 5, 2024
4 checks passed
@DevarshiChoudhury DevarshiChoudhury deleted the cython3 branch December 5, 2024 17:19
@evertrol
Copy link
Contributor

evertrol commented Dec 6, 2024

As a post scriptem for an attempted clarification about the actual error:

Cython will create shared object files from the *.pyx files that act as modules (though these are still wrapped in a 9-line *.py that loads the *.so file using importlib; not sure why that is being done that way). Each of these .so files will have a symbol (function) defined called PyInit_<module-name>, such as PyInit_hot_wrapper1. That is what Python expects, and how it identifies the module. For a package, it should be the package name instead of the module name, and the __init__.*.so file should contain such a symbol name. And it does indeed, e.g. PyInit_surface_radiation_field. But for some reason, Python itself expects the symbol to be named PyInit___init__ when loading the package; Python then doesn't find that specific symbol and it fails; this is what the error message shows: it can't find the PyInit___init__ symbol.

Why Python searches for PyInit___init__ and not for PyInit_<package-name> is a mystery to me. This does seem to be a bug in CPython on Windows, so not relevant here. Wild speculation is that the wrapper __init__.py script messes things up, and turns what should appear as a package into a standalone module (i.e., __init__.pyx by itself becomes a module, not a package starting point). As mentioned, I don't know why this wrapper exists. It appears to be constructed during installation, so perhaps it's a result of older-style installation, and the whole problem disappears with a newer-style installation. Again, this is wild speculation.

The advice I find on this topic on the internet (in the few sparse places that talk about this error), is to not put all kind of definitions in __init__.pyx, only just imports and keep it otherwise as barebones as possible, and put the (Cython) definitions in another file. In which case, the init file can just be a simple __init__.py Python file, instead of a Cython file. This is the workaround described by thjsal above. (The core.pyx naming scheme is borrowed from NumPy and AstroPy; anything similar like base.pyx obviously also works, but core.pyx/core.py is probably more recognisable.)

Footnotes

  1. find symbols in a dynamic library and the like with nm <file>.so.

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

Successfully merging this pull request may close these issues.

Newly released Cython may be backward incompatible for an X-PSI installation
4 participants