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

Build h5py against Cython < 3.0 in CI #2613

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

paulromano
Copy link
Contributor

Description

Some of you may have noticed that recent CI jobs are failing. It looks like this is due to a new release of Cython (3.0), which appears to be incompatible with the most recent release of h5py (see h5py/h5py#2268). This PR restricts the Cython version used to build h5py to get around that.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@shimwell
Copy link
Member

Just wondering if we need to limit the cython version in the pyproject.toml file as well

@paulromano
Copy link
Contributor Author

@shimwell Hopefully not since we're not doing anything super exotic in OpenMC with Cython, but we'll find out shortly when CI runs complete

@gridley
Copy link
Contributor

gridley commented Jul 19, 2023

This is a bummer because the newer cython version is required to circumvent a compile error that prevents it from working with python 3.11. Details below.

openmc/data/_endf.c: In function ‘__Pyx_AddTraceback’:
openmc/data/_endf.c:456:62: error: invalid use of incomplete typedef ‘PyFrameObject’ {aka ‘struct _frame’}
  456 |   #define __Pyx_PyFrame_SetLineNumber(frame, lineno)  (frame)->f_lineno = (lineno)
      |                                                              ^~
openmc/data/_endf.c:1903:5: note: in expansion of macro ‘__Pyx_PyFrame_SetLineNumber’
 1903 |     __Pyx_PyFrame_SetLineNumber(py_frame, py_line);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
error: command '/usr/bin/gcc' failed with exit code 1
gpad%

This has been encountered elsewhere and is apparently fixed in newer cython versions.

yaml/pyyaml#630

What do you all think of dropping cython altogether? The small amount of cython code could be translated to C++ with ease and made callable in openmc.lib. This might be a good task for an undergrad to get warmed up to coding with.

I say this because Cython is a complicated and apparently breakable dependency, whereas ctypes is rock solid.

@shimwell
Copy link
Member

shimwell commented Jul 19, 2023

I like that idea @gridley I think dropping cython would also help make openmc pip installable and in general keeping the dependency stack small is just good all around

I think in the meantime this gets the CI running again so I think we should merge

@paulromano
Copy link
Contributor Author

@gridley Cython is a build-time dependency. Note that this PR does not change the Cython version in our pyproject.toml, so in principle, you should still be able to build OpenMC against the newer Cython even if you have an h5py installation that was built against an older Cython.

@paulromano paulromano merged commit 8101328 into openmc-dev:develop Jul 20, 2023
@paulromano paulromano deleted the h5py-cython-workaround branch July 20, 2023 13:24
stchaker pushed a commit to stchaker/openmc that referenced this pull request Oct 25, 2023
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.

3 participants