Skip to content

Fix building sdist into wheels and avoid broken Cython#1915

Merged
ischoegl merged 3 commits intoCantera:mainfrom
bryanwweber:bryan-fix-cython-version
Jun 24, 2025
Merged

Fix building sdist into wheels and avoid broken Cython#1915
ischoegl merged 3 commits intoCantera:mainfrom
bryanwweber:bryan-fix-cython-version

Conversation

@bryanwweber
Copy link
Copy Markdown
Member

@bryanwweber bryanwweber commented Jun 22, 2025 •
edited
Loading

Changes proposed in this pull request

  • [Cython] Enforce cython cannot be v3.1.2
  • [Sdist] Template requirements versions

Building wheels is currently broken due to the regression fixed for CI in #1908 here and cython/cython#6957 in Cython. Since this version will always be broken, we should enforce in the build systems that it is incompatible.

If applicable, fill in the issue number this pull request is fixing

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@bryanwweber
[Cython] Enforce cython cannot be v3.1.2
05773cd 
This version of Cython has a bug with respect to compiling multiple modules independently. It was fixed for CI in Cantera#1908 and this change also enforces the version requirement for the build systems.
@bryanwweber
[Sdist] Template requirements versions
624d61f 
Sets template variables for dependency versions for the sdist. This will ensure consistent versioning across all our packages.

This change will also resolve errors building the sdist into wheels due to the regression fixed by cython/cython#6957
ischoegl
ischoegl reviewed Jun 22, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Thanks for addressing, @bryanwweber! Apart from the minor comment, could you have a look at the post-merge-tests that are currently failing due to the same Cython issue?

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 22, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.11%. Comparing base (4b7a3d6) to head (efdff2b).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1915   +/-   ##
=======================================
  Coverage   74.11%   74.11%           
=======================================
  Files         445      445           
  Lines       55454    55454           
  Branches     9121     9121           
=======================================
  Hits        41101    41101           
  Misses      11262    11262           
  Partials     3091     3091

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bryanwweber
Copy link
Copy Markdown
Member Author

bryanwweber commented Jun 22, 2025

Proof that the changes work to prevent using this broken Cython with Scons: https://github.com/Cantera/cantera/actions/runs/15809549593/job/44558991192?pr=1915#step:7:375 🎉

ERROR: Cython is an incompatible version: Found 3.1.2 but !=3.1.2,>=0.29.31 is required.

Now I just have to fix that it wasn't supposed to do that 😞

@bryanwweber
Copy link
Copy Markdown
Member Author

bryanwweber commented Jun 22, 2025 •
edited
Loading

Any ideas how to resolve this error when building on ubuntu:rolling or ubuntu:devel?

https://github.com/Cantera/cantera/actions/runs/15809741771/job/44559424078?pr=1915#step:7:252

g++ -o .sconf_temp/conftest_ce2dd1e38acb22ad88224c4d2c5f1d43_0.o -c -std=c++17 -D_GLIBCXX_ASSERTIONS -pthread -O3 -Wno-inline -DNDEBUG .sconf_temp/conftest_ce2dd1e38acb22ad88224c4d2c5f1d43_0.cpp
In file included from /usr/include/sundials/sundials_direct.h:23,
                 from /usr/include/cvodes/cvodes_ls.h:21,
                 from /usr/include/cvodes/cvodes.h:20,
                 from .sconf_temp/conftest_ce2dd1e38acb22ad88224c4d2c5f1d43_0.cpp:3:
/usr/include/sundials/sundials_types.h:57:10: fatal error: mpi.h: No such file or directory
   57 | #include <mpi.h>
      |          ^~~~~~~
compilation terminated.
scons: Configure: no

I think it's this error: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1082085 but I don't recall if it's come up here before.

@ischoegl
Copy link
Copy Markdown
Member

ischoegl commented Jun 22, 2025

Any ideas how to resolve this error when building on ubuntu:rolling or ubuntu:devel?

https://github.com/Cantera/cantera/actions/runs/15809741771/job/44559424078?pr=1915#step:7:252

g++ -o .sconf_temp/conftest_ce2dd1e38acb22ad88224c4d2c5f1d43_0.o -c -std=c++17 -D_GLIBCXX_ASSERTIONS -pthread -O3 -Wno-inline -DNDEBUG .sconf_temp/conftest_ce2dd1e38acb22ad88224c4d2c5f1d43_0.cpp
In file included from /usr/include/sundials/sundials_direct.h:23,
                 from /usr/include/cvodes/cvodes_ls.h:21,
                 from /usr/include/cvodes/cvodes.h:20,
                 from .sconf_temp/conftest_ce2dd1e38acb22ad88224c4d2c5f1d43_0.cpp:3:
/usr/include/sundials/sundials_types.h:57:10: fatal error: mpi.h: No such file or directory
   57 | #include <mpi.h>
      |          ^~~~~~~
compilation terminated.
scons: Configure: no

I think it's this error: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1082085 but I don't recall if it's come up here before.

I have the suspicion that it’s related to #1843. It’s obviously unrelated to Cython …

ischoegl
ischoegl reviewed Jun 22, 2025
View reviewed changes
@bryanwweber
Copy link
Copy Markdown
Member Author

bryanwweber commented Jun 23, 2025 •
edited
Loading

I have the suspicion that it’s related to #1843. It’s obviously unrelated to Cython …

Yes thank you, I knew I'd seen it here before!

Edit: it looks like Ray fixed it with the mpi* compiler wrappers, so I'll do that too.

@bryanwweber
Copy link
Copy Markdown
Member Author

bryanwweber commented Jun 24, 2025

Edit: it looks like Ray fixed it with the mpi* compiler wrappers, so I'll do that too.

After talking to Ingmar offline, I think we need a little more discussion about how to handle this changed dependency requirement, instead of just forcing mpi* compilers. I'm going to defer the work to a separate PR to resolve #1843 specifically

@bryanwweber bryanwweber requested review from ischoegl and speth June 24, 2025 10:23
@bryanwweber
[CI] Fix some of the post-merge tests
efdff2b 
This change fixes several failures in the post-merge tests:

* Disallow Cython 3.1.2 to avoid the duplicate symbols bug
* Resolve Python dependency conflicts on Python 3.14

Aside from the fixes, this change adds:

* uv pip install for package installs from PyPI. uv is much faster than
  pip while still being compatible
* Add post_merge_reqs.txt, a requirements.txt-format file to store
  dependencies needed to build and test Cantera in the post-merge test
  workflow. Using this file allows the dependencies to be cached,
  reducing overall runtime when versions don't change. Hopefully this
  method can be extended for other workflows to reduce churn in the
  history when dependencies need to be updated.
@bryanwweber bryanwweber force-pushed the bryan-fix-cython-version branch from 4a72ccc to efdff2b Compare June 24, 2025 10:35
ischoegl
ischoegl approved these changes Jun 24, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Thanks, @bryanwweber !

@ischoegl ischoegl merged commit dee0d51 into Cantera:main Jun 24, 2025
58 of 60 checks passed
@bryanwweber bryanwweber deleted the bryan-fix-cython-version branch June 24, 2025 13:05
@speth
Copy link
Copy Markdown
Member

speth commented Jun 24, 2025

Edit: it looks like Ray fixed it with the mpi* compiler wrappers, so I'll do that too.

After talking to Ingmar offline, I think we need a little more discussion about how to handle this changed dependency requirement, instead of just forcing mpi* compilers. I'm going to defer the work to a separate PR to resolve #1843 specifically

I think we probably need to start a discussion upstream with the SUNDIALS devs or the Ubuntu package maintainers. There should be some way of making MPI optional so it's not necessary to compile a serial program with the MPI compiler wrapper.

@ischoegl ischoegl mentioned this pull request Jun 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ischoegl ischoegl ischoegl approved these changes

@speth speth Awaiting requested review from speth

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bryanwweber @ischoegl @speth