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

Add 3.12 to action, but without cython support #2294

Merged
merged 6 commits into from Jan 16, 2024

Conversation

Ericgig
Copy link
Member

@Ericgig Ericgig commented Jan 15, 2024

Description
Add python 3.12 to automated test and wheel build.
However python 3.12 removed the imp module used by pyximport.
The fix on cython side may only be for cython 3.X, so we run without cython at runtime on 3.12.

Related to #2293

Comment on lines 126 to 127
# Builds without Cython at runtime. This is a core feature;
# everything should be able to run this.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the comment to mention why we have an extra non-Cython run for 3.12.

warnings.warn(
"The new version of Cython, (>= 3.0.0) is not supported."
.format(_Cython.__version__)
)
elif _version2int(_Cython.__version__.split()[0]) >= _version2int("3.12.0"):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this elif condition. We appear to be comparing a Python version to a Cython version?

return
import qutip
if not qutip.settings.has_cython:
pytest.skip("Cython not available at runtime.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit uncomfortable relying on QuTiP's own detection magic to skip tests. If it ends up being wrong in some cases Cython tests will be silently skipped in CI.

Perhaps let's just check against the Python version and skip if it's Python 3.12?

@coveralls
Copy link

coveralls commented Jan 15, 2024

Coverage Status

coverage: 70.427%. first build
when pulling 37a6f44 on Ericgig:bug.312_cy
into 7a883d3 on qutip:qutip-4.7.X.

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Left one more comment, otherwise looks good.

qutip/tests/conftest.py Outdated Show resolved Hide resolved
Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
@Ericgig Ericgig merged commit 1423eb4 into qutip:qutip-4.7.X Jan 16, 2024
12 of 13 checks passed
@Ericgig Ericgig deleted the bug.312_cy branch January 16, 2024 15:14
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.

None yet

3 participants