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

[core] runtime context resource ids getter #26907

Merged
merged 9 commits into from
Jul 24, 2022

Conversation

richardliaw
Copy link
Contributor

Why are these changes needed?

We’re seeing a deprecation message from XGBoost Ray:
DeprecationWarning: ray.worker.get_resource_ids is a private attribute and access will be removed in a future Ray version.
and we use this function call in two places for XGboost:
To set the right OMP_NUM_THREADS per actor (=num cpus assigned to the actor).
Configuring the right number of threads for the XGBoost model (=num cpus per worker).

The current workaround will be to thread the num_cpus_per_worker configuration from the driver through all the actors, but ideally (1) would be configured for us.

Related issue number

Checks

  • 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 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 :(

Signed-off-by: Richard Liaw <[email protected]>
Signed-off-by: Richard Liaw <[email protected]>
@richardliaw richardliaw added the core-interface-change-approval-required This changes the Ray core behavior / API and requires broader approvals. label Jul 22, 2022
@richardliaw
Copy link
Contributor Author

Note that we should absolutely get this in for 2.0 (to clean up the XGBoost experience).

@scv119
Copy link
Contributor

scv119 commented Jul 22, 2022

seems reasonable to me. @jjyao @rkooo567 take another look?
Is there any public doc we need update?

python/ray/runtime_context.py Outdated Show resolved Hide resolved
"""Get the assigned resources to this worker.

Returns:
A dictionary mapping the name of a resource to a list of pairs, where
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should return a dict mapping from resource name to count. Id of the resource is only meaningful for GPU and we already have a ray.worker.get_gpu_ids() for that purpose. Also feel we should copy get_gpu_ids() into runtime context as well for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I updated and added a docstring. Can you take a look?

I also won't address the get_gpu_ids() thing since we should probably have a broader discussion there (get_gpu_ids() is frequently used in multiple libraries?)

@jjyao jjyao added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jul 22, 2022
Signed-off-by: Richard Liaw <[email protected]>
@richardliaw richardliaw removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jul 23, 2022
@richardliaw richardliaw merged commit d01a80e into ray-project:master Jul 24, 2022
krfricke pushed a commit to ray-project/xgboost_ray that referenced this pull request Jul 25, 2022
Following ray-project/ray#26907, we can now avoid a deprecation message when using XGBRay with 2.0.

Signed-off-by: Richard Liaw <[email protected]>
Rohan138 pushed a commit to Rohan138/ray that referenced this pull request Jul 28, 2022
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-interface-change-approval-required This changes the Ray core behavior / API and requires broader approvals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants