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

Remove nbytes_in_memory #4930

Merged
merged 1 commit into from
Jun 18, 2021
Merged

Conversation

mrocklin
Copy link
Member

This doesn't appear to be necessary for any scheduling decisions today, and is causing issues. Let's remove it.

This doesn't appear to be necessary
@mrocklin
Copy link
Member Author

git blame puts the blame on me on this one. This was added as part of the original TaskGroup/TaskPrefix PR, which did carry the sin of speculative work. My apologies to all.

@gjoseph92
Copy link
Collaborator

I was thinking of a similar solution 😄

What about nbytes_total? I'm not sure what that's supposed to mean, but it might have similar problems.

I could see nbytes_in_memory become useful in the TaskGroup visualization, but if we actually want it we can add it back for that.

@mrocklin
Copy link
Member Author

I recommend that we wait on merging this PR until after the release today, just in case someone was depending on this internal state.

@mrocklin
Copy link
Member Author

In the meantime, cherry-picking this PR might be a good idea :)

@mrocklin mrocklin merged commit d9bc3c6 into dask:main Jun 18, 2021
@mrocklin mrocklin deleted the remove-nbytes-in-memory branch June 18, 2021 17:29
@jakirkham
Copy link
Member

Also happy to see this go away. I don't think it was used anywhere

@jakirkham
Copy link
Member

Out of curiosity what issues was it causing?

@jrbourbeau
Copy link
Member

This happened to pop up and cause issues in a couple of places over the past few days. #4925 (comment) is one of those places. #4927 has a nice writeup from @gjoseph92

@gjoseph92
Copy link
Collaborator

#4927, #4927 (comment), #4925 (comment). Oddly, after a long period of dormancy, a few of us all ran into this from different angles at the same time.

@jakirkham
Copy link
Member

Ok thanks all 😄

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.

4 participants