-
Notifications
You must be signed in to change notification settings - Fork 26.3k
[ez] Explicit env for run_test #120251
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
[ez] Explicit env for run_test #120251
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/120251
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New FailuresAs of commit 419c0fb with merge base b33e8d3 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot merge -f "should be fine" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
This PR removes and adds some failures and successes that were hidden in the past week (ish). #119408 (47182a8) accidentally removed environment variables on rerun (see PR body of #120251 for slightly more details). Enabling testing with dynamo is set using an env var, so if a test failed with dynamo, it would rerun without the dynamo env var set, making it pass on retry. Normally, the flaky test bot would catch this and make an issue for the test, but the CI env var controls whether or not xml test reports get made, and that also got removed on rerun, so the xmls weren't made either. Pull Request resolved: #120271 Approved by: https://github.com/DanilBaibak, https://github.com/zou3519
| extra_unittest_args=None, | ||
| env=None, | ||
| ) -> int: | ||
| env = env or os.environ.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you just set the desired default instead of None: env=os.environ.copy()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously I'd have linters be unhappy about using mutable objects as default arguments in functions, something about how changes can persist. I don't know if copying the env would fix the problem, so I did it this way to be safe
env=None (which is the default) inherits the env from the calling process. Explicitly set the env to the calling process env so that things can be added to it later
Tested in: https://hud.pytorch.org/pytorch/pytorch/commit/e7b4d8ec880c56883d684fcc05b6cdb20452aafb
Checked that test-reports (which depend on the CI env var) get made.