[core] [1/2] Topology aware scheduling public API - #63479
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Signed-off-by: aaron.li <aaron.li@anyscale.com>
| from ray.util.placement_group import placement_group, placement_group_table | ||
|
|
||
| NODE_ID_LABEL = "ray.io/node-id" | ||
| RACK_LABEL = "rack_id" |
There was a problem hiding this comment.
nit: I'd rather keep the rack label examples as "ray.io/gpu-domain" to be consistent with prior docs
|
|
||
| cluster.add_node(num_cpus=4, labels=rack1_labels) | ||
|
|
||
| def assert_pg_nodes_label_value(cluster_nodes, pg, label, value): |
There was a problem hiding this comment.
this is only validating the rack level right, can we do validation for the node level as well?
|
|
||
|
|
||
| def test_topology_strategy_strict_spread(ray_start_cluster): | ||
| """Testing STRICT_SPREAD on the node level and STRICT_PACK on the rack level""" |
There was a problem hiding this comment.
same here, let's validate that STRICT_SPREAD actually works
| #: The node label key used for label-domain scheduling | ||
| #: (e.g. "ray.io/gpu-domain"). Empty string if the placement group | ||
| #: does not use label-domain scheduling. | ||
| #: Per-level topology strategy for this placement group. Each level is a |
There was a problem hiding this comment.
I think mentioning "per-level" is a bit confusing as we only support a flat hierarchy right now. Would probably leave it out for now.
|
|
||
| Raises: | ||
| ValueError: if bundle type is not a list. | ||
| ValueError: if empty bundle or empty resource bundles are given. | ||
| ValueError: if the wrong lifetime arguments are given. | ||
| ValueError: if `topology_strategy` and `strategy` are both provided, | ||
| or if `topology_strategy` does not match the v1 shape. |
There was a problem hiding this comment.
what do you mean by "v1 shape"?
| effective_strategy = "PACK" | ||
|
|
||
| stripped_level = {k: v for k, v in level.items() if k != NODE_ID_LABEL_KEY} | ||
| topology_strategy_for_core = [stripped_level] if stripped_level else [] |
There was a problem hiding this comment.
"for_core" this is all core code and it doesn't tell me what this represents. The variable should tell us how this is different from the topology_strategy variable. Also I don't quite understand what effective_strategy means, maybe call this "node_level_strategy" instead?
There was a problem hiding this comment.
Can't think of a good name, stripped_topology_strategy will have to suffice
| f"`topology_strategy[0]['{NODE_ID_LABEL_KEY}']`." | ||
| ) | ||
|
|
||
| # Resolve the effective strategy for the rest of validation that depends on it. |
There was a problem hiding this comment.
I would be a bit more specific and state that this is the case where topology_strategy is not set and we're doing this to support the legacy default, i.e. default pack for the node level strategy
There was a problem hiding this comment.
Hmm, good catch, I think there is even more underlying issue with node level strategy not being set properly.
Sparks0219
left a comment
There was a problem hiding this comment.
Can you also update the documentation at placement-group.rst
Signed-off-by: aaronscalene <aaron.li@anyscale.com>
| </TableCell> | ||
| <TableCell align="center"> | ||
| {label_domain_key ? ( | ||
| {topology_strategy && |
There was a problem hiding this comment.
i agree with your point: validation of our temporary definition only happens in one place
|
|
||
| NODE_ID_LABEL = "ray.io/node-id" | ||
| RACK_LABEL = "ray.io/gpu-domain" | ||
| ONE = "rack-1" |
|
|
||
| # Six rack-1 nodes for four bundles — STRICT_SPREAD has slack. | ||
| for _ in range(6): | ||
| cluster.add_node(num_cpus=1, labels=rack1_labels) |
| assert_pg_nodes_label_value(ray.nodes(), pg, RACK_LABEL, ONE) | ||
|
|
||
|
|
||
| def test_topology_strategy_infeasible_after_node_kill(ray_start_cluster): |
There was a problem hiding this comment.
yes, will get rid of this.
| #: (e.g. "ray.io/gpu-domain"). Empty string if the placement group | ||
| #: does not use label-domain scheduling. | ||
| #: The topology strategy for this placement group. Each level is a | ||
| #: dict mapping a label key (e.g. "rack_id", "ray.io/gpu-domain") to a |
|
|
||
|
|
||
| def test_topology_strategy_reschedule_on_node_failure(ray_start_cluster): | ||
| """Verify rescheduling stays within the same topology level on partial failure. |
| {label_domain_assignments && | ||
| Object.keys(label_domain_assignments).length > 0 | ||
| ? JSON.stringify(label_domain_assignments) | ||
| {topology_assignments && |
There was a problem hiding this comment.
nit: same comment here as above
There was a problem hiding this comment.
Sorry, missed this. Done.
There was a problem hiding this comment.
Great job, lgtm! @MengjinYan could you do a pass as well and sign off on this?
|
@aaronscalene could you also attach a screenshot of what the updated dashboard with topology aware scheduling looks like + the cli output as well? |
Signed-off-by: aaron.li <aaron.li@anyscale.com>
MengjinYan
left a comment
There was a problem hiding this comment.
Looks good overall! Mostly nits and questions.
| "STRICT_SPREAD", | ||
| } | ||
|
|
||
| NODE_ID_LABEL_KEY = "ray.io/node-id" |
There was a problem hiding this comment.
Nit: This is already defined here:
ray/python/ray/includes/common.pxi
Line 166 in e36060c
I think we should use it directly.
There was a problem hiding this comment.
Seems like I cannot use it directly. According to AI, _raylet imports placement_group.py for some reason, which then causes a cyclic dependency if we want to use _raylet.RAY_NODE_ID_KEY. This causes compilation error for the ray binary.
There was a problem hiding this comment.
Make sense. If that's the case, can you add comments in common.pxi as well to make sure those values will always be in sync
| ) | ||
|
|
||
|
|
||
| def _derive_node_level_strategy(strategy: str, topology_strategy: Dict[str, str]): |
There was a problem hiding this comment.
Return type should be added to the function
| _validate_bundle_label_selector(bundle_label_selector) | ||
|
|
||
| if strategy not in VALID_PLACEMENT_GROUP_STRATEGIES: | ||
| if node_level_strategy not in VALID_PLACEMENT_GROUP_STRATEGIES: |
There was a problem hiding this comment.
Super nit: In the case where the node_level_strategy is derived from the topology_strategy, the check here seems redundant. Probably better to move the check together with _validate_topology_strategy.
| placement group on a per-bundle level. | ||
| topology_strategy: Topology-aware placement. A dict mapping each | ||
| topology label key to a placement strategy (e.g. | ||
| ``{"ray.io/node-id": "STRICT_PACK", "rack_id": "STRICT_PACK"}``). |
There was a problem hiding this comment.
Nit: for the Ray labels, we should add ray.io prefix. In the example, I think it could be better to use ray.io/gpu-domain instead of rack_id.
| soft_target_node_id, | ||
| c_vector[unordered_map[c_string, c_string]] bundle_label_selector): | ||
| c_vector[unordered_map[c_string, c_string]] bundle_label_selector, | ||
| topology_strategy): |
There was a problem hiding this comment.
Better to make the topology_strategy typed.
There was a problem hiding this comment.
Nit: it seems that not all the "label_domain" terminology hasn't been updated with "topology_strategy". It could be better to unify the terms to avoid confusion.
There was a problem hiding this comment.
It seems that the scheduling_result.selected_label_domain in this file still needs to be updated. At the same time, there are other files still contains the old "label domain" naming. They might worth to be cleaned up as well. For the change, I'm okay with putting it to a separate PR.
There was a problem hiding this comment.
Sorry, I was careless with this one. Thanks for the pushback as always. I agree with an additional PR though, since it seems the scope of the rename is larger than we thought. I will leave a TODO for this issue (#64370) and push a PR asap for that change once this gets merged.
| std::optional<std::string> label_domain = placement_group.GetLabelDomainKey(); | ||
| if (label_domain.has_value()) { | ||
| const std::string &label_domain_key = label_domain.value(); | ||
| // Currently, take the first topology key as the label domain the scheduler should |
There was a problem hiding this comment.
Just to clarify, with this PR, we only support specifying 1 key for the topology strategy, is that correct? If yes, might worth a comment clarify it to avoid confusion.
There was a problem hiding this comment.
Yes, cleared this up, an artifact of the past.
|
@aaronscalene Let's go! Can't wait to get this in! |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 2aeea5e. Configure here.
# Conflicts: # python/ray/_raylet.pyx
MengjinYan
left a comment
There was a problem hiding this comment.
Thanks! The terminology clean up can be in a separate PR.
|
Thank you @MengjinYan @Sparks0219 and @Yicheng-Lu-llll ❤️ 😄 |
## Description This is directly related to #63479. This is the necessary docs change for the change in the previous PR. This describes how the topology aware scheduling alpha feature works. --------- Signed-off-by: aaron.li <aaron.li@anyscale.com> Signed-off-by: aaronscalene <aaron.li@anyscale.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
## Description This is directly related to [this REP](ray-project/enhancements#66). This commit introduces topology aware scheduling for the public API, providing users a public-facing API to use as opposed to the code added in ray-project#61442. For more information, please read the REP and the private facing API. ## Considerations ### API Validation Currently, our API will only support one layer of topology aware scheduling. Furthermore, we currently only support at max one node level placement strategy and label aware placement strategy. We also only support STRICT_PACK currently on label aware placement strategies. Thus, our `_validate_topology_strategy` will mainly focus on ensuring that users are only allowed to pass in these inputs. ### Node Level Placement Strategy Whenever `topology_strategy` is defined, we will not allow users to specify `strategy` and instead only be able to specify strategy through the label `ray.io/node-id`. We enforce this through our validation as well. However, currently, this `ray.io/node-id` is just extracted from the user input and piped as the `strategy` input anyways into the code. In the future, we will have to consider a more robust way to pipe this input through once we decided to support multiple levels as well as multiple labels to be scheduled on per level. ### Label Locality Private Functionality Currently, we support `bundle_label_selectors` to be specified with GB200 / 300 to implicitly call into the same topology aware code. Within this, we are deprecating this functionality in support of this PR, which supports a more general version. Although we considered that this will deprecate parts of the API we support, we decided to continue with this more general version since we believe that having one path is more intuitive for future users. Furthermore, since our initial feature is just an alpha feature, we are comfortable with removing it. ## Testing New unit tests added, and existing unit tests are translated over from GB200 / 300 support. The new dashboard UI looks like below: <img width="1610" height="108" alt="image" src="https://github.com/user-attachments/assets/0b442cfc-6ff1-4765-9423-279a1a6aa264" /> The new `ray list placement-groups --detail`: ``` - placement_group_id: 94cbd842f8deeee2167e746f1cc101000000 name: '' creator_job_id: '01000000' state: CREATED ... topology_strategy: - entries: ray.io/gpu-domain: STRICT_PACK topology_assignments: - assignments: ray.io/gpu-domain: rack-1 ... ``` --------- Signed-off-by: aaron.li <aaron.li@anyscale.com> Signed-off-by: aaronscalene <aaron.li@anyscale.com> Co-authored-by: Aaron <35825663+aarli421@users.noreply.github.com>
…oject#63740) ## Description This is directly related to ray-project#63479. This is the necessary docs change for the change in the previous PR. This describes how the topology aware scheduling alpha feature works. --------- Signed-off-by: aaron.li <aaron.li@anyscale.com> Signed-off-by: aaronscalene <aaron.li@anyscale.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Description
This is directly related to this REP. This commit introduces topology aware scheduling for the public API, providing users a public-facing API to use as opposed to the code added in #61442. For more information, please read the REP and the private facing API.
Considerations
API Validation
Currently, our API will only support one layer of topology aware scheduling. Furthermore, we currently only support at max one node level placement strategy and label aware placement strategy. We also only support STRICT_PACK currently on label aware placement strategies. Thus, our
_validate_topology_strategywill mainly focus on ensuring that users are only allowed to pass in these inputs.Node Level Placement Strategy
Whenever
topology_strategyis defined, we will not allow users to specifystrategyand instead only be able to specify strategy through the labelray.io/node-id. We enforce this through our validation as well. However, currently, thisray.io/node-idis just extracted from the user input and piped as thestrategyinput anyways into the code. In the future, we will have to consider a more robust way to pipe this input through once we decided to support multiple levels as well as multiple labels to be scheduled on per level.Label Locality Private Functionality
Currently, we support
bundle_label_selectorsto be specified with GB200 / 300 to implicitly call into the same topology aware code. Within this, we are deprecating this functionality in support of this PR, which supports a more general version. Although we considered that this will deprecate parts of the API we support, we decided to continue with this more general version since we believe that having one path is more intuitive for future users. Furthermore, since our initial feature is just an alpha feature, we are comfortable with removing it.Testing
New unit tests added, and existing unit tests are translated over from GB200 / 300 support.
The new dashboard UI looks like below:

The new
ray list placement-groups --detail: