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

Garbage collect Scheduler Instances during testing #6281

Closed
wants to merge 20 commits into from

Conversation

sjperkins
Copy link
Member

@sjperkins sjperkins commented May 5, 2022

Closes #6250

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

@sjperkins sjperkins marked this pull request as draft May 5, 2022 16:35
distributed/utils_test.py Outdated Show resolved Hide resolved
distributed/utils_test.py Outdated Show resolved Hide resolved
distributed/utils_test.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

Unit Test Results

       16 files  ±0         16 suites  ±0   8h 13m 38s ⏱️ + 28m 2s
  2 767 tests ±0    2 686 ✔️ ±0       78 💤  - 2  3 +2 
22 098 runs  ±0  21 075 ✔️  - 1  1 020 💤  - 1  3 +2 

For more details on these failures, see this check.

Results for commit 11de7b7. ± Comparison against base commit 8411c2d.

♻️ This comment has been updated with latest results.

with pristine_loop() as loop:
with check_process_leak(check=processes):
with check_instances() if instances else nullcontext():
with check_instances() if instances else nullcontext():
Copy link
Member Author

Choose a reason for hiding this comment

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

The IOLoop needs to be cleaned up first, otherwise callbacks on the loop like Scheduler.remove_worker will hold onto Scheduler instances and cause check_instances to fail.

I'm not aware of any drawbacks to this ordering.

s = next(iter(Scheduler._instances))
import objgraph

objgraph.show_backrefs([s], filename="scheduler.png")
Copy link
Member Author

Choose a reason for hiding this comment

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

The objgraph import should be removed upon PR approval

@@ -6697,7 +6697,7 @@ async def get_story(self, keys=()):

transition_story = story

def reschedule(self, key=None, worker=None):
def reschedule(self, key=None, worker=None, stimulus_id=None):
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a smaller PR addressing this here: #6307

@@ -30,7 +30,7 @@ async def test_profile_plot(c, s, a, b):
assert len(p.source.data["left"]) >= 1


@gen_cluster(client=True, clean_kwargs={"threads": False})
@gen_cluster(client=True, clean_kwargs={"threads": False, "instances": False})
Copy link
Member Author

Choose a reason for hiding this comment

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

instance checks are disabled on a variety of tests largely due to #6308.

Garbage collecting Scheduler objects is useful, but secondary relative to clearing up TaskStates and their expensive run_spec attributes.

We should log an issue that tracks re-enabling them.

@fjetter fjetter closed this Jun 15, 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.

Ensure TaskState instances are released on Scheduler and Worker
2 participants