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 post-merge tests #1752

Merged
merged 6 commits into from
Aug 2, 2024
Merged

Conversation

bryanwweber
Copy link
Member

@bryanwweber bryanwweber commented Aug 1, 2024

Changes proposed in this pull request

  • Set up the SpecifierSets for version comparisons to allow pre-release versions of packages. This shouldn't change any behavior for stable versions.
  • hdf variables no longer need to be specified to SCons CLI, they're handled by SConstruct since Configure libhdf5_serial if it's available #1723
  • Install pre-release wheels from the anaconda index for NumPy, SciPy, Pandas, and Cython. This means we don't have to build them ourselves 🎉
  • If the Python package is built for a newer version of Python than we support, ignore pip's error about unsupported Python because the user is warned by SCons anyways
  • Add support for Python 3.13 to the extensions

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

Closes #1751

If applicable, provide an example illustrating new features this pull request is introducing

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 bryanwweber force-pushed the bryan-fix-post-merge-tests branch from 57a32e7 to a395f8d Compare August 1, 2024 16:20
@@ -50,7 +50,7 @@ jobs:
- name: Build Cantera
run: python3 `which scons` build env_vars=all
CXX=clang++-14 CC=clang-14 f90_interface=n extra_lib_dirs=/usr/lib/llvm/lib
-j4 debug=n --debug=time logging=debug
-j4 debug=n --debug=time logging=debug python_package=y
Copy link
Member

Choose a reason for hiding this comment

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

I've been wondering if we should update the default to build the Python package unless it is explicitly disabled, so that missing a Python dependency results in a build failure unless you set python_package=n.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm for it! Do you want to make an enhancements issue so we can at least try to document some tradeoffs?

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.12%. Comparing base (886dfc1) to head (57e1164).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1752      +/-   ##
==========================================
- Coverage   73.13%   73.12%   -0.01%     
==========================================
  Files         381      381              
  Lines       54134    54134              
  Branches     9222     9224       +2     
==========================================
- Hits        39590    39587       -3     
- Misses      11587    11589       +2     
- Partials     2957     2958       +1     

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

@bryanwweber bryanwweber force-pushed the bryan-fix-post-merge-tests branch from fb79b2e to ec109f9 Compare August 1, 2024 23:12
@bryanwweber bryanwweber self-assigned this Aug 2, 2024
@bryanwweber bryanwweber force-pushed the bryan-fix-post-merge-tests branch 2 times, most recently from b7004de to 9d9580e Compare August 2, 2024 14:00
@bryanwweber bryanwweber force-pushed the bryan-fix-post-merge-tests branch from 9d9580e to 8736cbc Compare August 2, 2024 14:13
We've already warned the user that their Python may be unsupported, so try the wheel build with pip anyways.
Set up unit tests to be skipped if pint can't be imported. There is a known bug with pint and Python 3.13 that prevents importorskip from working.
@bryanwweber bryanwweber force-pushed the bryan-fix-post-merge-tests branch from 8736cbc to d039b1f Compare August 2, 2024 14:17
@bryanwweber bryanwweber marked this pull request as ready for review August 2, 2024 14:23
@bryanwweber bryanwweber requested a review from a team August 2, 2024 14:23
@bryanwweber
Copy link
Member Author

As of yesterday, Pandas pre-release wheels were available for Python 3.13: https://github.com/Cantera/cantera/actions/runs/10207129578/job/28266033217 They are no longer available right now 😞

@bryanwweber
Copy link
Member Author

I also don't think I did anything that would start causing the sample tests to fail. It does look like the artifact uploaded of libcantera_shared is not correct, it's only a few hundred bytes as opposed to the kB that are uploaded on main. 🆘

@speth
Copy link
Member

speth commented Aug 2, 2024

It's uploading the symlink, not the actual file the symlink points to. Seems to be an unexpected change introduced in the latest version of upload-artifact, as reported in actions/upload-artifact#589.

@bryanwweber
Copy link
Member Author

That was my guess, that it was uploading the symlink. Glad to know it's not just us. I'll pin to 4.3.4 for now.

@bryanwweber
Copy link
Member Author

Based on pandas-dev/pandas#59372, it looks like wheels get removed after some time in the nightly index, probably to avoid overloading Anaconda's willingness to provide storage 😬 And as it happens, Pandas wheels haven't been uploading for a while, due to test failures on their part. So I'm fine with accepting those jobs failing.

@bryanwweber bryanwweber force-pushed the bryan-fix-post-merge-tests branch from f825b51 to 6b2c8bb Compare August 2, 2024 20:54
Version 4.3.5 broke uploading symlinks as artifacts. Reverting to 4.3.4 should resolve this until the error is fixed.
@bryanwweber bryanwweber force-pushed the bryan-fix-post-merge-tests branch from 6b2c8bb to 57e1164 Compare August 2, 2024 20:55
Copy link
Member

@speth speth 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. This all looks fine to me.

@speth speth merged commit 520267d into Cantera:main Aug 2, 2024
56 of 58 checks passed
@bryanwweber bryanwweber deleted the bryan-fix-post-merge-tests branch August 4, 2024 12:43
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.

Post-merge tests are failing, but can be fixed 🎉
2 participants