mods to enable python3.12

[mefuller]

Changes proposed in this pull request

• enable building Cantera with Python 3.12 (thanks for the pointer @speth)
• update docs in source to include Python 3.12
• verified in Fedora 39, Rawhide builds (e.g. https://koji.fedoraproject.org/koji/buildinfo?buildID=2279235)

Closes #1602

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

[ischoegl]

Thanks, @mefuller! Too bad that Python 3.12 isn't available on the regular channels yet; it is definitely true that Python 3.12 needs to be covered by CI if we enable this.

There is one option I see (if we truly want to live on the bleeding edge), which is to use a fedora:rawhide container. The following GH action should work:

  fedora-runner:
    name: Fedora rawhide test
    runs-on: ubuntu-latest
    container:
      image: fedora:rawhide
    steps:
    - uses: actions/checkout@v3
    - name: Install dependencies
      run: |
        dnf -y install <all the cantera dependencies>
    ...

Other than adding this to .github/workflows/main.yaml, an alternative would be to just test in .github/workflows/post-merge-tests.yaml, which would be a great middle-ground. (PS: for review purposes, it should probably be added in main first, before moving it to post-merge-tests prior to merging)

PPS: see #1608, which implements the idea for Ubuntu containers

[bryanwweber]

3.12 release candidates are usually available from the setup-python action, if it helps. Since conda won't get binaries (and of dependencies) until the final release, everything will have to come from pip anyways.

[speth]

Other than adding this to .github/workflows/main.yaml, an alternative would be to just test in .github/workflows/post-merge-tests.yaml, which would be a great middle-ground. (PS: for review purposes, it should probably be added in main first, before moving it to post-merge-tests prior to merging)

I agree that this would be better in the post-merge-tests.yaml. I'm wondering if there's a way to modify this workflow to be able to manually run it on a PR / fork. My best guess is that we'd need to add another input to the workflow_dispatch section, and then modify the checkout action to specify that fork instead of defaulting to the base repository.

[ischoegl]

I agree that this would be better in the post-merge-tests.yaml. I'm wondering if there's a way to modify this workflow to be able to manually run it on a PR / fork. My best guess is that we'd need to add another input to the workflow_dispatch section, and then modify the checkout action to specify that fork instead of defaulting to the base repository.

I have a container-based CI run almost ready to go in #1608 (using ubuntu images; it's easy to go to different linux flavors from there). Unfortunately, I'm not familiar with manually-triggered runs though.

[ischoegl]

3.12 release candidates are usually available from the setup-python action, if it helps. Since conda won't get binaries (and of dependencies) until the final release, everything will have to come from pip anyways.

@bryanwweber ... it's unfortunately not available yet, which is why I suggested the Fedora runner. This btw also relates to Cantera/enhancements#37. PS: just found one of your posts mentioning deadsnakes, which would be an alternative.

[mefuller]

@ischoegl is there any additional information you'd need from me on running the build in Fedora? I'm happy to help

For reference, the RPM spec file (which generates the packaged binaries) is found at https://src.fedoraproject.org/rpms/cantera/blob/rawhide/f/cantera.spec

That file has a complete list of packages required for building and the exact arguments passed to scons

[bryanwweber]

@ischoegl The docs definitely mention allowing pre-release versions: https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md (all the way at the bottom, sorry I'm on my phone so I can't directly link). This should be orthogonal to adding container-based actions, though

[bryanwweber]

My best guess is that we'd need to add another input to the workflow_dispatch section, and then modify the checkout action to specify that fork instead of defaulting to the base repository.

If a PR is open for a branch, the merged commit (I believe this means "as though the PR branch was merged into the base branch") is available in the base repository without needing to specify the fork. I'd have to look up how to get that commit, though... 😕

[ischoegl]

@ischoegl The docs definitely mention allowing pre-release versions: https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md (all the way at the bottom, sorry I'm on my phone so I can't directly link). This should be orthogonal to adding container-based actions, though

Yes, it is orthogonal. @mefuller actually used the setup-python action in this PR, but the runner failed. I believe the critical information is on the docs you linked (the allow-prerelease: true bit) 🎉

      - uses: actions/setup-python@v4
        with:
          python-version: "${{ matrix.python_version }}"
          allow-prereleases: true

@mefuller ... I believe easiest path forward for this PR is to add the option to the setup-python action above; once 3.12 is covered by CI, the PR itself is ready to merge. I still believe it would make sense to add a post-merge-test for fedora eventually (although I won't do this myself: this is likely more of an interest to the fedora community). They are relatively easy to set up, as demonstrated in #1608.

[speth]

I don't think we should be testing any pre-release software as part of the main.yml workflow that runs on every update to a PR -- post-merge-tests.yml is a more appropriate place for such tests. In this case, such builds will be slower because they will need to compile dependencies (notably, numpy) from source. And in general, such builds are much more likely to have failures that have nothing to do with Cantera, and I think that's just an unnecessary source of noise when trying to evaluate a PR.

Given the release schedule for Python 3.12 (planned for October 2), that either means moving this job to the other workflow, or waiting until then to merge this PR.

In either case, I'm 👍 on adding a Rawhide container build to post-merge-tests.yml, though that's perhaps separate from this PR.

[ischoegl]

Thanks, @mefuller for the PR! While the changes themselves are thankfully modest, the main sticking point are tests that are currently failing.

While I am only speaking for myself, I'd like to see the newly added Python 3.12 support covered by some CI. As was pointed out, there are convincing arguments against testing pre-release versions of Python in main GH actions.

This leaves tests of Python 3.12 in a rawhide container in the post-merge-tests phase. As a workable template already exists in #1608, it would be great if a similar container-based CI test could be included here (it probably makes sense to wait until the other PR is reviewed/merged to avoid merge conflicts).

[ischoegl]

Per @speth's comment, please remove '3.12', unless you want to wait for a couple of months.

[bryanwweber]

Why not duplicate this job into post-merge with only 3.12? Then we could avoid changing Python version and OS in the same job, to better isolate build failures.

[ischoegl]

🤷‍♂️ ... either way. I believe @speth's point was that many dependencies will have to be compiled. The rawhide container should provide for those dependencies already. As both rawhide and 3.12 are pre-release, having post-merge-tests fail is just a heads-up, where most failures are probably due to upstream issues and should wash out over time. Personally, I'm voting for the container, as it's testing Python (and everything else!) in the wild. It's probably the more informative test for the Fedora community. The fact that the current Fedora rawhide failure is due to Python is imho just coincidental.

[speth]

You could add both in the post-merge tests. I'm not even sure which would be more likely to succeed. The logs in #1602 seem to indicate that the only reason that this doesn't work on Python 3.12 in that case is because Cantera is explicitly ignoring it.

[bryanwweber]

Yes, I was trying to suggest having both. Apologies that wasn't clear.

where most failures are probably due to upstream issues and should wash out over time.

I actually doubt this would be the case, I would be willing to bet there's a good amount of signal in those failures. I think the main point to have this in post-merge (which I agree with) is to avoid blocking a PR over failures in pre-release software.

[ischoegl]

@ischoegl is there any additional information you'd need from me on running the build in Fedora? I'm happy to help

For reference, the RPM spec file (which generates the packaged binaries) is found at https://src.fedoraproject.org/rpms/cantera/blob/rawhide/f/cantera.spec

That file has a complete list of packages required for building and the exact arguments passed to scons

@mefuller ... now that #1608 is merged, could you rebase? Running tests in a Fedora container, - rawhide and/or latest - should be very simple to set up (likewise for Python, but I'm less of a Pythonista than @bryanwweber). There's only a couple of lines that need to be edited from the ubuntu version (see commit 9ee13eb). Let me know if you have questions, I'd be happy to help!

As an aside, I had a look over the dependencies; you may want to consider the new/optional libhdf5 dependency for your Fedora builds (Cantera 3.0 and later) as we deprecated/removed h5py support (Fedora likely doesn't have the odd install location that ubuntu is using); I also noticed that you're not explicitly listing any blas/lapack libraries (although the Cantera installer may find something that is implicit), which works but likely isn't the best option (for the Cantera conda channel; we use openblas on Linux systems). Other than that, scipy is only used for examples, i.e. optional.

PS: I created a runner here: 81c1058 although I have not tested whether it works. Feel free to cherry-pick!

[mefuller]

@ischoegl I made the requested/suggested changes with the exception of HDF5
HDF5 support is something I want to get into the Fedora Cantera build, but I need to get some other maintainers to update a few dependencies
OpenBLAS is an implicit requirement since it's pulled in by other deps in the spec file (this is considered good form), but in the next Fedora build I will add system_blas_lapack=y to the scons build for good housekeeping

[ischoegl]

Thanks, @mefuller! This is almost there. I'd just have a few more requests:

1. could you remove the '3.12' from line 64 in main.yaml as indicated above? That way, we won't have failing tests.
2. temporarily copy fedora-docker over to main.yaml in a new commit, and push so CI runs a test. This is to see whether the job is configured correctly
3. once everything runs, remove the test from main.yaml again (by dropping the commit), so things are ready to merge

This will allow us to review the output of the run on the Actions tab. Ideally, all of the tests (both main and post-merge-tests) should run successfully at this point.

Regarding HDF5, it presumably will compile on fedora? Testing this is a question that is likely separate from packaging decisions, but I'll leave this up to you. Fwiw, the HighFive package we're using is header only, so it is no longer needed once things are compiled.

[ischoegl]

Thanks, @mefuller 🎉 … tests look great! While Python 3.12 adds some syntax warnings for raw strings in SConstruct, this is not preventing compilation from failing and may go beyond this PR. I’ll approve from my side once the last commit is dropped.

[mefuller]

@ischoegl test commit dropped

[ischoegl]

@mefuller ... thanks again. Regarding hdf5, it looks like fedora actually packages highfive-devel, but it's rather dated (2 years old), see https://packages.fedoraproject.org/pkgs/highfive/highfive-devel/fedora-rawhide.html and https://github.com/BlueBrain/HighFive/releases/tag/v2.3.1 ... is this what you were referring to?

[mefuller]

@ischoegl exactly - I'm on it
See https://bugzilla.redhat.com/show_bug.cgi?id=2236670