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

Fix test isolation for stateful configurations in the testsuite #9403

Merged
merged 6 commits into from Jan 26, 2024

Conversation

sklam
Copy link
Member

@sklam sklam commented Jan 24, 2024

This fixes a SIGILL detected in conda-forge/numba-feedstock#131 by adding needed isolation to test that changes the target machine.

Minimal reproducer is:

python runtests.py numba.tests.test_vectorization.TestVectorization.test_nditer_loop numba.tests.test_withlifting.TestLiftObj.test_case02_mutate_array_ahead_of_ctx

@sklam sklam added this to the 0.59.0 milestone Jan 24, 2024
@sklam sklam added the skip_release_notes Skip towncrier requirement label Jan 24, 2024
@sklam sklam closed this Jan 24, 2024
@sklam sklam reopened this Jan 24, 2024
@sklam
Copy link
Member Author

sklam commented Jan 25, 2024

I have confirmed that it passes with conda-forge build-locally.py for a previously failing config.

@stuartarchibald
Copy link
Contributor

@sklam suggest changing the PR title to something like "Fix test isolation for stateful configurations" or something like that?

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @sklam. I checked the test suite for uses of override_env_config where an overridden env var is likely to impact machine code generation, it seems like this patch isolates all remaining cases which weren't already covered. As noted OOB there's still a potential issue with the use of the fast flag, but this is better resolved through fixing #7168. There's one minor comment to resolve else looks good. Thanks again!

numba/tests/test_caching.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: l4604 of this file has overrides.append(override_env_config(k, v)) which, by virtue of its use, is also creating skylake ISA specific machine code, however, all the tests in this test class are in subprocesses so it is "safe".

Copy link
Member Author

Choose a reason for hiding this comment

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

@stuartarchibald stuartarchibald added the 4 - Waiting on author Waiting for author to respond to review label Jan 26, 2024
@sklam sklam changed the title Fix SIGILL Fix test isolation for stateful configurations in the testsuite Jan 26, 2024
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@sklam sklam marked this pull request as ready for review January 26, 2024 14:49
@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jan 26, 2024
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the update. This is approved conditional on CI passing once main is stable again.

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jan 26, 2024
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Re-approve following merge of main into this branch.

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Jan 26, 2024
@sklam sklam merged commit e7d072b into numba:main Jan 26, 2024
21 checks passed
@sklam sklam deleted the fix/sigill branch January 26, 2024 19:18
sklam added a commit to sklam/numba that referenced this pull request Jan 26, 2024
Fix test isolation for stateful configurations in the testsuite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge skip_release_notes Skip towncrier requirement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants