Simplify Python extension loading and other Python improvements by speth · Pull Request #1456 · Cantera/cantera

speth · 2023-03-18T01:11:11Z

Changes proposed in this pull request

Simplify loading of Python extension support by letting Cython handle the complexity of loading the extension module after Python has already been initialized. In the case of a non-Python main program, all we have to do is initialize Python and import the Cantera Python module.
Add support for Cython 3.0.0b1
Convert C++ NotImplementedError to Python native NotImplementedError
Improve documentation of _SolutionBase._references

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

Let Cython handle the complexity of loading the extension module after Python has already been initialized. In the case of a non-Python main program, all we have to do is initialize Python and import the Cantera Python module.

codecov · 2023-03-18T01:50:44Z

Codecov Report

Merging #1456 (e2d6d23) into main (0c5fc1d) will increase coverage by 0.06%.
The diff coverage is 90.38%.

@@            Coverage Diff             @@
##             main    #1456      +/-   ##
==========================================
+ Coverage   69.86%   69.92%   +0.06%     
==========================================
  Files         373      377       +4     
  Lines       56684    57262     +578     
  Branches    18883    19151     +268     
==========================================
+ Hits        39600    40043     +443     
- Misses      14571    14669      +98     
- Partials     2513     2550      +37

Impacted Files	Coverage Δ
interfaces/cython/cantera/mixture.pyx	`90.47% <ø> (ø)`
interfaces/cython/cantera/reactor.pyx	`90.54% <ø> (ø)`
interfaces/cython/cantera/solutionbase.pyx	`89.66% <ø> (ø)`
include/cantera/cython/funcWrapper.h	`70.83% <50.00%> (-0.36%)`	⬇️
interfaces/cython/cantera/delegator.pyx	`79.91% <89.74%> (+1.09%)`	⬆️
interfaces/cython/cantera/_onedim.pyx	`79.76% <100.00%> (+0.27%)`	⬆️
interfaces/cython/cantera/composite.py	`89.16% <100.00%> (+0.09%)`	⬆️
interfaces/cython/cantera/func1.pyx	`100.00% <100.00%> (ø)`
interfaces/cython/cantera/thermo.pyx	`92.35% <100.00%> (ø)`

... and 26 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

interfaces/cython/cantera/thermo.pyx

ischoegl

@speth ... this looks great overall. The only item I'd request would be a todo comment to move sentinels to C++ after Cantera 3.0.

speth · 2023-03-18T23:21:39Z

I created an issue (#1457) to track the needed feature. I think that's preferable to a to-do item buried somewhere in the code.

ischoegl

I created an issue (#1457) to track the needed feature. I think that's preferable to a to-do item buried somewhere in the code.

Thanks for creating the issue! I agree that it is preferable to a todo.

It did, however, occur to me that SolutionArray needs a similar sentinel, somewhere around here:

cantera/interfaces/cython/cantera/composite.py

Lines 558 to 559 in fbfd93f

    
           def __init__(self, phase, shape=(0,), states=None, extra=None, meta={}, init=True): 
        
               self._phase = phase

Even prior to the introduction of C++ SolutionArray, adding species would have created inconsistencies in stored data.

PS: Another item I noticed is that we may want to test with Cython 3.0 again?

Currently used for testing pre-release versions of Cython

speth · 2023-03-19T17:17:37Z

I've added the Python weakref-style checks to both SolutionArray and Quantity.

I've also added a new CI job that runs only after pushes to main, and tests whatever the current pre-release version of Cython is. This creates a home for some of the tests that we've discussed in Cantera/enhancements#37, where running them on every push to a PR is more trouble than it's worth. I tested it by pushing this branch directly to Cantera/cantera. You can see the status (green checkmark) for this job reported here: https://github.com/Cantera/cantera/tree/python-ext-init. I'll delete that branch after we merge this PR.

ischoegl

Thanks, @speth … everything looks good to me!

speth added 4 commits March 16, 2023 21:58

Simplify Python extension initialization

1991bcd

Let Cython handle the complexity of loading the extension module after Python has already been initialized. In the case of a non-Python main program, all we have to do is initialize Python and import the Cantera Python module.

Add support for Cython 3.0.0b1

e043fe6

Convert C++ NotImplementedError to Python NotImplementedError

bd69754

Improve documentation of _SolutionBase._references

02fce07

speth added the Python label Mar 18, 2023

ischoegl reviewed Mar 18, 2023

View reviewed changes

interfaces/cython/cantera/thermo.pyx Show resolved Hide resolved

ischoegl requested changes Mar 18, 2023

View reviewed changes

speth mentioned this pull request Mar 18, 2023

addSpecies should be disabled on ThermoPhase objects in use by reactors and flames #1457

Closed

ischoegl requested changes Mar 19, 2023

View reviewed changes

speth added 2 commits March 19, 2023 12:18

[Python] SolutionArray and Quantity block add_species

294695c

[CI] Add CI tests that run only after a PR is merged to main

e2d6d23

Currently used for testing pre-release versions of Cython

ischoegl approved these changes Mar 19, 2023

View reviewed changes

ischoegl merged commit 3cbe8aa into Cantera:main Mar 19, 2023

speth deleted the python-ext-init branch March 19, 2023 22:00

speth mentioned this pull request Mar 21, 2023

Improve CI Infrastructure support for Cantera Cantera/enhancements#37

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Simplify Python extension loading and other Python improvements#1456

Simplify Python extension loading and other Python improvements#1456
ischoegl merged 6 commits intoCantera:mainfrom
speth:python-ext-init

speth commented Mar 18, 2023 •

edited

Loading

Uh oh!

codecov bot commented Mar 18, 2023 •

edited

Loading

Uh oh!

Uh oh!

ischoegl left a comment

Uh oh!

speth commented Mar 18, 2023

Uh oh!

ischoegl left a comment •

edited

Loading

Uh oh!

speth commented Mar 19, 2023

Uh oh!

ischoegl left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	def __init__(self, phase, shape=(0,), states=None, extra=None, meta={}, init=True):
	self._phase = phase

Uh oh!

Conversation

speth commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

ischoegl left a comment

Choose a reason for hiding this comment

Uh oh!

speth commented Mar 18, 2023

Uh oh!

ischoegl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

speth commented Mar 19, 2023

Uh oh!

ischoegl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

speth commented Mar 18, 2023 •

edited

Loading

codecov bot commented Mar 18, 2023 •

edited

Loading

ischoegl left a comment •

edited

Loading