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

Refactor threading layer priority tests to not use stdout/stderr #7749

Merged
merged 5 commits into from
Jan 26, 2022

Conversation

stuartarchibald
Copy link
Contributor

As title, this prevents output from e.g diagnostics being able to
influence the outcome of the tests.

Fixes #7458

As title, this prevents output from e.g diagnostics being able to
influence the outcome of the tests.

Fixes numba#7458
@stuartarchibald stuartarchibald added this to the Numba 0.55.1 milestone Jan 17, 2022
@esc
Copy link
Member

esc commented Jan 26, 2022

Although the PR didn't touch this line, I would recommend to change:

env = os.environ.copy()

into

env = {}

Copying the entire environment is a bit dangerous. For tests, it is better to start with a clean slate to avoid whatever variables exist in the environment to leak into the test. May not become an issue in practice, but I believe it's prudent to make sure it really doesn't.

Copy link
Member

@esc esc left a comment

Choose a reason for hiding this comment

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

It looks great. A few minor things and it'll be good to go!

numba/tests/test_parallel_backend.py Show resolved Hide resolved
numba/tests/test_parallel_backend.py Show resolved Hide resolved
@esc esc added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jan 26, 2022
@stuartarchibald
Copy link
Contributor Author

Although the PR didn't touch this line, I would recommend to change:

env = os.environ.copy()

into

env = {}

Copying the entire environment is a bit dangerous. For tests, it is better to start with a clean slate to avoid whatever variables exist in the environment to leak into the test. May not become an issue in practice, but I believe it's prudent to make sure it really doesn't.

Agree, am happy to make this change whilst editing these tests. It was, however, the env copy that actually triggered and demonstrated the problem with these tests in the first place!

@stuartarchibald stuartarchibald 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, 2022
esc
esc previously approved these changes Jan 26, 2022
Copy link
Member

@esc esc left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, looks good now!

@esc esc added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jan 26, 2022
@stuartarchibald
Copy link
Contributor Author

Turns out that doing #7749 (comment) leads to:

Fatal Python error: _Py_HashRandomization_Init: failed to get random numbers to initialize Python
Python runtime state: preinitialized

on windows, probably because the sysroot is cleared. 1d7f47b puts back the copy of the env as it's likely the easiest way to obtain a suitable environment that doesn't require tracking required Python env vars.

@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 5 - Ready to merge Review and testing done, is ready to merge labels Jan 26, 2022
Copy link
Member

@esc esc left a comment

Choose a reason for hiding this comment

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

OK, let's go with the original.

@esc esc added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jan 26, 2022
@sklam sklam merged commit b126abc into numba:master Jan 26, 2022
esc pushed a commit to esc/numba that referenced this pull request Jan 27, 2022
Refactor threading layer priority tests to not use stdout/stderr
esc pushed a commit to esc/numba that referenced this pull request Jan 27, 2022
Refactor threading layer priority tests to not use stdout/stderr
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 Effort - short Short size effort needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor unit tests scraping stdout/stderr in cases where alternatives are available.
3 participants