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

[3.12] gh-110276: No longer ignore PROFILE_TASK failure silently (#110295) #111950

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 10, 2023

@vstinner
Copy link
Member Author

We should not ignore test failure when running the PGO build: see the new failure #111929 impacting 3.11 and 3.12 branches.

@vstinner
Copy link
Member Author

Once merged, this change should be backported to the 3.11 branch once #111949 is merged.

@vstinner
Copy link
Member Author

cc @AlexWaygood

@vstinner
Copy link
Member Author

@Yhg1s: Are you ok with this change? Previously, the build couldn't fail. But in exchange, we can miss real bugs. See #110276 and #111929 for two recent PGO bugs.

@vstinner
Copy link
Member Author

Oh, the initial issue was #101634

@vstinner
Copy link
Member Author

@gpshead: 3.11 and 3.12 are also affected. Are you fine with my change?

@gpshead
Copy link
Member

gpshead commented Nov 13, 2023

yeah backporting that makes sense.

@Yhg1s
Copy link
Member

Yhg1s commented Nov 13, 2023

No, I would rather not backport this change. The job of the profiling run isn't to check whether tests pass or not. It's to exercise enough of Python and the standard library to make a reasonable profile. A test failure doesn't mean that didn't happen. Even not running any tests doesn't mean there is no reasonable profile. I don't think it's enough of a problem to warrant breaking existing 3.12 build processes.

Making the profile task test failures fatal in 3.13 is fine, but I'd rather not disrupt anyone's existing 3.12 builds and make upgrading to 3.12.1 that much harder.

@vstinner
Copy link
Member Author

@Yhg1s:

Making the profile task test failures fatal in 3.13 is fine, but I'd rather not disrupt anyone's existing 3.12 builds and make upgrading to 3.12.1 that much harder.

@AlexWaygood: Python 3.12 has spoken, I close this PR ;-)

@vstinner
Copy link
Member Author

If tomorrow, this issue becomes a major issue, maybe we can consider adding an option to regrtest to return a non-zero exit code if --pgo tests fail. I did that for "env changed": I added --fail-env-changed, option enabled by --fast-ci and --slow-ci used by buildbots and pre-commit CIs. By default, "env changed" are not reported as errors (by default, exit code is zero: success).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants