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

Split RAM measure into dask keys/other old/other new #4651

Merged
merged 79 commits into from Apr 20, 2021

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Mar 30, 2021

Closes #4634

DONE: backend
DONE: frontend
DONE: backend unit tests
DONE: frontend unit tests
DONE: stress testing
Out of scope (left to later PRs): use these measures in heuristics
Out of scope (left to later PRs): frontend design for >= 50 workers

Note: please switch to incognito mode or wipe your cache before testing this PR. If the dashboard from the main branch is in your browser's cache, bokeh will misbehave.

@@ -361,6 +447,8 @@ class WorkerState:
_last_seen: double
_local_directory: str
_memory_limit: Py_ssize_t
_memory_other_history: "deque[tuple[float, Py_ssize_t]]"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sadly, Cython doesn't seem to support from __future__ import annotations yet; it would have made the quotes unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

I think it will in Cython 3 ( cython/cython#3829 ). That's probably still a ways off though

@crusaderky
Copy link
Collaborator Author

@jacobtomlinson see how you like it now

@jacobtomlinson
Copy link
Member

jacobtomlinson commented Apr 19, 2021

I see no change with that. I wonder if this is a scaling thing. High density displays are scaled at 2x. Setting those numbers to 100px and 120px displays nicely on my screen.

Edit I tested on my 1920 x 1200 primary display and also saw no change, but 100/120 displayed well.

@crusaderky
Copy link
Collaborator Author

I don't get it - this is how 1920x1080 looks like in my screen with 60px (using chrome dev tools):

Screenshot from 2021-04-19 16-41-06

is this a problem of you having a much larger font?

@crusaderky
Copy link
Collaborator Author

@jacobtomlinson try now...

@jacobtomlinson
Copy link
Member

No that also didn't do anything.

I notice looking at yours that the plots don't have any axis labels. Can you try adding some workers and checking it again?

I am just testing with python -c 'from dask.distributed import LocalCluster, Client; client = Client(); from time import sleep; sleep(3600);'.

@crusaderky
Copy link
Collaborator Author

retested height with the axis labels

@jacobtomlinson
Copy link
Member

Perfect!

image

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for iterating with me on the status page @crusaderky.

Everything else looks good to me.

@jacobtomlinson
Copy link
Member

Let's get this in. It looks like all feedback has been addressed and it's been open for a while now. If there is anything outstanding we can tweak it in follow-up PRs.

Thanks for your efforts here @crusaderky!

@jacobtomlinson jacobtomlinson merged commit d5fc324 into dask:main Apr 20, 2021
@crusaderky crusaderky deleted the other_mem branch April 20, 2021 13:15
@mrocklin
Copy link
Member

mrocklin commented Apr 20, 2021 via email

@jrbourbeau
Copy link
Member

jrbourbeau commented Apr 20, 2021

It looks like test_memory is failing fairly consistently both here and in the main branch

@crusaderky
Copy link
Collaborator Author

I'm onto it. It WAS working. :$

@crusaderky
Copy link
Collaborator Author

Fix in #4719

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.

Visualize split between dask keys and memory leaks/other
7 participants