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

also run dataclasses test with optimizations enabled #275

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

matthiasdiener
Copy link
Contributor

@matthiasdiener matthiasdiener commented Dec 4, 2024

Please squash

@matthiasdiener
Copy link
Contributor Author

With these changes, we perhaps won't need a way to provide a frozen_override option like in #274.

@matthiasdiener
Copy link
Contributor Author

matthiasdiener commented Dec 4, 2024

By the way, the Py3.x CI is currently testing 3.12 (which thus gets tested twice); should we change this to Py3.13?

@alexfikl
Copy link
Contributor

alexfikl commented Dec 4, 2024

By the way, the Py3.x CI is currently testing 3.12 (which thus gets tested twice); should we change this to Py3.13?

That's weird. I thought Python 3.13 was available on Github. Any idea why it's not picked up?
https://github.com/actions/python-versions/blob/main/versions-manifest.json

@matthiasdiener
Copy link
Contributor Author

matthiasdiener commented Dec 4, 2024

By the way, the Py3.x CI is currently testing 3.12 (which thus gets tested twice); should we change this to Py3.13?

That's weird. I thought Python 3.13 was available on Github. Any idea why it's not picked up? https://github.com/actions/python-versions/blob/main/versions-manifest.json

Yeah, it is available. But for some reason Py3.x defaults to 3.12 on Ubuntu (MacOS gets 3.13 already 🤷 ).

Edit: Perhaps they can't decide whether to use freethreading or not? 🤣

@inducer
Copy link
Owner

inducer commented Dec 4, 2024

By the way, the Py3.x CI is currently testing 3.12 (which thus gets tested twice); should we change this to Py3.13?

Nah, they'll update it soon enough. Don't have time to track Github's Python version shenanigans in detail.

@inducer inducer merged commit c3fc7f0 into inducer:main Dec 4, 2024
17 checks passed
@matthiasdiener matthiasdiener deleted the opt-frozen-override branch December 4, 2024 20:10
@@ -97,6 +97,10 @@ jobs:
curl -L -O https://gitlab.tiker.net/inducer/ci-support/raw/main/build-and-test-py-project.sh
. ./build-and-test-py-project.sh

# Also run with optimizations turned on, since opt_frozen_dataclass
# depends on the __debug__ setting.
python -O -m pytest pytools/test/test_dataclasses.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but how does this work? I thought -O disabled assertions, but pytest does some assertion hackery to make the tests fail and whatnot, so combining them seems very odd.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah! Great question. I had not thought about this. It used to be the case that pytest got upset when you ran it with python -O, but that's (clearly!) no longer the case. So something else must be happening. A litte bit of digging turns up that pytest rewrites the AST to swap out assert statements for something entirely different, which, IIUC, makes it OK for pytest to run with python -O.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how pytest re-enables assertions when running with -O, but I think they rewrite them to provide more information? (see e.g. https://github.com/pytest-dev/pytest/blob/9d4f36d87dae9a968fb527e2cb87e8a507b0beb3/src/_pytest/assertion/rewrite.py)

They seem to work fine as long as they occur in the test module. See this warning from CI:

=============================== warnings summary ===============================
.env/lib/python3.12/site-packages/_pytest/config/__init__.py:1278
  /home/runner/work/pytools/pytools/.env/lib/python3.12/site-packages/_pytest/config/__init__.py:1278: PytestConfigWarning: assertions not in test modules or plugins will be ignored because assert statements are not executed by the underlying Python interpreter (are you using python -O?)
  
    self._warn_about_missing_assertion(mode)

We set -O not because of assertions, but to modify the value of __debug__, which is what opt_frozen_dataclass looks at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very cool! Thanks! I'll read those more carefully to see what they're doing there 😁

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.

3 participants