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

Mock process memory readings in test_worker.py (v2) #5878

Merged
merged 20 commits into from
Mar 1, 2022

Conversation

crusaderky
Copy link
Collaborator

In scope

Out of scope

  • same treatment to test_scheduler.py::test_memory
  • remediation to test_active_memory_manager.py, which should be carried out by adding a switch to use managed memory
  • remediation to all tests around rebalance(): postponed indefinitely due to the whole function to be reimplemented on top of the Active Memory Manager

@github-actions
Copy link
Contributor

Unit Test Results

       12 files  ±0         12 suites  ±0   7h 24m 25s ⏱️ - 3m 58s
  2 621 tests +1    2 540 ✔️ +3    80 💤 ±0  1  - 2 
15 650 runs  +6  14 784 ✔️ +5  864 💤 +2  2  - 1 

For more details on these failures, see this check.

Results for commit 3484f0c. ± Comparison against base commit fb8484e.

Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

I think these tests are much more readable and easier to maintain, thank you!

Comment on lines +1361 to +1363
while not a.data.disk or len(a.data.disk) > prev_n:
prev_n = len(a.data.disk)
await asyncio.sleep(0)
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit fragile. As soon as the while loop in the memory monitor changes anything about how it awaits, e.g. another async call to evict data, this condition will break since we rely on skipping exactly one loop iteration with sleep(0).

I can't come up with a nicer way other than sleeping for a longer time so this is fine for me. I just wanted to raise this in case somebody else thinks of something

Copy link
Member

Choose a reason for hiding this comment

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

Another ugly thing we can do but I think I dislike this even more is to wait for _memory_monitoring to toggle since then we know for sure the memory_monitor is done. I think I prefer sticking with sleep(0) and deal with a test failure/test rewrite if that ever happens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_spill_hysteresis flaky on ubuntu
2 participants