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

schema: Memory thresholds should never be exactly 0.0. #7458

Merged
merged 1 commit into from
Jan 6, 2023

Conversation

stuarteberg
Copy link
Member

@stuarteberg stuarteberg commented Jan 6, 2023

In the config, memory thresholds such as distributed.worker.memory.terminate should never be exactly 0.0.
Instead, config should use false to disable memory management.

This one bit me recently. My older dask config files used 0.0 to disable the memory management features. That worked because older versions of distributed interpreted the value 0.0 to be equivalent to false for these fields. But in newer versions, only false works. (I suspect the change occurred in #5904.)

Nowadays, if the config says 0.0, then distributed interprets that literally -- and no memory can be used at all without incurring the wrath of the memory manager!

An easy "fix" is to disallow 0.0 in the user's config. In json schema, exclusiveMinimum: 0 ensures that the value 0.0 itself is not permitted by the schema.


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

Instead, config should use 'false' to disable memory management
@mrocklin
Copy link
Member

mrocklin commented Jan 6, 2023

Hi @stuarteberg 👋

@crusaderky I suspect that you understand this topic better than I do. Would you be comfortable owning this issue?

@crusaderky
Copy link
Collaborator

crusaderky commented Jan 6, 2023

This makes sense to me. It avoids us having to set up over-complicated use case tests between 0 and False.

@stuarteberg stuarteberg marked this pull request as ready for review January 6, 2023 19:38
@stuarteberg
Copy link
Member Author

@crusaderky The CI has sporadic failures, but I think they're unrelated to this change. I'm not sure if there's anything else to do in this PR. (Do schema changes require unit tests?)

Unless you have any suggestions, I think this is good for review/merge.

@mrocklin mrocklin merged commit 3e793f7 into dask:main Jan 6, 2023
@mrocklin
Copy link
Member

mrocklin commented Jan 6, 2023

Thanks @stuarteberg !

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.

3 participants