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

No longer double count transfer cost in stealing #7036

Merged
merged 10 commits into from
Sep 16, 2022

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Sep 15, 2022

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

@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2022

Unit Test Results

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

       15 files  ±  0         15 suites  ±0   6h 11m 48s ⏱️ - 1m 25s
  3 107 tests +  6    3 017 ✔️ +  2    85 💤 ±0  5 +4 
22 997 runs  +43  22 086 ✔️ +39  906 💤 ±0  5 +4 

For more details on these failures, see this check.

Results for commit 9c9438e. ± Comparison against base commit 1fd07f0.

♻️ This comment has been updated with latest results.

@hendrikmakait hendrikmakait marked this pull request as ready for review September 16, 2022 06:46
Comment on lines +250 to +251
assert ts.processing_on
assert ts in ts.processing_on.long_running
Copy link
Member

Choose a reason for hiding this comment

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

Codecov says this is not hit but I'd expect this to be hit by test_dont_steal_long_running_tasks

We should probably look at that test, maybe some test assumption is no longer true

Copy link
Member

Choose a reason for hiding this comment

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

Ok, test_dont_steal_long_running_tasks is pretty meaningless. I believe something went wrong during a refactoring of that test. It only asserts that long_tasks are only "executed" once, i.e. they are not rescheduled. This is entirely irrelevant for stealing.

Copy link
Member

Choose a reason for hiding this comment

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

Given that this test was already this way I suggest to set this out of scope for this PR. I might be able to address this with #7030

@fjetter
Copy link
Member

fjetter commented Sep 16, 2022

All failing tests are known offenders except of test_wait_first_completed but I opened #7039 to make that one more robust

@hendrikmakait
Copy link
Member Author

@fjetter fjetter merged commit e892d0b into dask:main Sep 16, 2022
@hendrikmakait hendrikmakait mentioned this pull request Oct 4, 2022
2 tasks
gjoseph92 pushed a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022
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.

Stealing balance not accounting for recent decisions
2 participants