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

[Data] Allow specifing both num_cpus and num_gpus for map APIs #47995

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

scottjlee
Copy link
Contributor

@scottjlee scottjlee commented Oct 11, 2024

Why are these changes needed?

Currently, we enforce that num_cpus and num_gpus cannot be both set for map operations. This PR enables the user to specify both of these parameters, as we believe that with the recent improvements to the scheduler, Ray Data should be able to smoothly support this scenario. However, we will warn users that this is still an experimental feature.

Updated doc example (updated for all map APIs):
Screenshot at Oct 11 19-00-23

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Copy link
Member

@bveeramani bveeramani left a comment

Choose a reason for hiding this comment

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

OOC -- do you know why we can't set both?

@scottjlee
Copy link
Contributor Author

OOC -- do you know why we can't set both?

@bveeramani according to the docstring:

Namely, args must explicitly specify either CPU or GPU, not both. Disallowing
mixed resources avoids potential starvation and deadlock issues during scheduling,
and should not be a serious limitation for users.

@raulchen
Copy link
Contributor

actually, I'm not sure if this issue still exists with a lot of scheduler changes.
And some users do want to set both.
I was gonna suggest that we should probably remove this limitation and see what happens.

@scottjlee
Copy link
Contributor Author

@raulchen should we just go ahead and remove the restriction then? any other testing that i should do / concerns to be addressed?

@raulchen
Copy link
Contributor

@scottjlee maybe let's update the error to a warning first

@scottjlee scottjlee changed the title [Data] [Docs] Note that num_cpus and num_gpus cannot be both set for map operations [Data] Allow specifing both num_cpus and num_gpus for map APIs Oct 12, 2024
@raulchen raulchen enabled auto-merge (squash) October 15, 2024 18:19
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Oct 15, 2024
Signed-off-by: Scott Lee <[email protected]>
@scottjlee scottjlee enabled auto-merge (squash) October 15, 2024 19:33
@scottjlee scottjlee merged commit 5a65213 into ray-project:master Oct 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants