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 cleanup iteration in save_sys_modules #8713

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Jun 20, 2024

When iterating sys.path, then if you delete index i, all elements >i will now be one less than what was put into enumerate. So we should remove elements in reverse to prevent indices getting out of sync.

When iterating sys.modules, then modifying it may break the .keys() iterator, so work on a copy of the necessary keys.

  • Tests added / passed
  • Passes pre-commit run --all-files

When iterating `sys.path`, then if you delete index `i`, all elements
>`i` will now be one less than what was put into `enumerate`. So we
should remove elements in reverse to prevent indices getting out of
sync.

When iterating `sys.modules`, then modifying it _may_ break the
`.keys()` iterator, so work on a copy of the necessary keys.
@QuLogic QuLogic requested a review from fjetter as a code owner June 20, 2024 23:38
Copy link
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    29 files  ±0      29 suites  ±0   10h 59m 42s ⏱️ - 3m 14s
 4 061 tests ±0   3 956 ✅ ±0     97 💤 ±0  8 ❌ ±0 
55 939 runs  ±0  53 768 ✅ +5  2 162 💤 ±0  9 ❌  - 5 

For more details on these failures, see this check.

Results for commit 7cc808a. ± Comparison against base commit adcb045.

@fjetter fjetter merged commit 01e5ff3 into dask:main Jun 24, 2024
28 of 37 checks passed
@QuLogic QuLogic deleted the fix-module-iteration branch June 24, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants