Skip to content

Conversation

@clee2000
Copy link
Contributor

@clee2000 clee2000 commented Feb 20, 2024

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.

@pytorch-bot
Copy link

pytorch-bot bot commented Feb 20, 2024

🔗 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 Failures

As of commit 419c0fb with merge base b33e8d3 (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@clee2000 clee2000 marked this pull request as ready for review February 20, 2024 22:54
@clee2000 clee2000 requested a review from a team as a code owner February 20, 2024 22:54
@clee2000 clee2000 changed the title Explicit env for run_test [ez] Explicit env for run_test Feb 20, 2024
@clee2000
Copy link
Contributor Author

@pytorchbot merge -f "should be fine"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

pytorchmergebot pushed a commit that referenced this pull request Feb 21, 2024
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()
Copy link
Contributor

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()?

Copy link
Contributor Author

@clee2000 clee2000 Feb 22, 2024

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

@clee2000 clee2000 deleted the csl/explicit_env branch February 26, 2024 20:53
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.

5 participants