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

Define "GPU" as a worker resource #1401

Open
wants to merge 2 commits into
base: branch-24.12
Choose a base branch
from

Conversation

pentschev
Copy link
Member

Add "GPU" as a worker resource to each CUDA worker. This should support users in identifying whether the workers available contain a GPU resource.

@pentschev pentschev requested a review from a team as a code owner October 23, 2024 20:52
@github-actions github-actions bot added the python python code needed label Oct 23, 2024
resources = dict(pair.split("=") for pair in resources)
resources = valmap(float, resources)
gpu_resources = valmap(int, itemfilter(lambda x: x != "GPU", resources))
resources = valmap(float, itemfilter(lambda x: x == "GPU", resources))
Copy link
Member Author

Choose a reason for hiding this comment

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

@jacobtomlinson you have written this section originally, I presume mapping values to float was done to support a definition such as "MEMORY" but since I don't see any tests or any other explicit mention in Dask-CUDA, could you comment if that's right and whether you can think of more robust ways for us to handle types here other than what I wrote above as "GPU" or NOT "GPU"?

Copy link
Member

Choose a reason for hiding this comment

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

Honestly I don't remember the reasoning. I think dask handles these values as floats, as you say it's to support values like MEMORY or other arbitrary quantities.

@pentschev pentschev added 3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change labels Oct 24, 2024
@rjzamora
Copy link
Member

@pentschev - Sorry this fell through the cracks. Do you think this is good to go for 24.12?

@pentschev
Copy link
Member Author

@rjzamora I actually don't know how useful this is, @jacobtomlinson point out offline that tasks ignore annotations after the dask-expr work. Doing this seems like it may resolve the original ask, but at the same time it is very misleading to users who may attempt to use those annotations for their actual purpose with Dask. I don't have a strong opinion on not going ahead, so maybe @VibhuJawa and @ryantwolf who originally requested this functionality can comment on whether it solves their issue nevertheless and whether they think it's useful to introduce this even knowing it can mislead users (ourselves included).

@rjzamora
Copy link
Member

I don't have a strong opinion on not going ahead

Okay, fair enough. I'll just assume we are holding off on this unless the Curator team tells us otherwise.

@jacobtomlinson point out offline that tasks ignore annotations after the dask-expr work. Doing this seems like it may resolve the original ask, but at the same time it is very misleading to users who may attempt to use those annotations for their actual purpose with Dask.

Yes, annotations are not supported by dask-expr, and I don't personally expect that they will ever be supported. With that said, I do think worker-resources are still useful. In the long run, it may become possible to add the resource restriction to the new TaskSpec Task object. It's also possible to use a scheduler plugin to apply these resources for specific tasks (as done in #1398).

Doing this seems like it may resolve the original ask, but at the same time it is very misleading to users who may attempt to use those annotations for their actual purpose with Dask.

Users who try to apply annotations should get a warning. So, I don't think it hurts much if resources are labeled or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change python python code needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants