By the new semantics of the bundles field, I think the field should be empty in the beginning. And probably same with the strategy if we add it to the placement group scheduling options

Tried adding this and it runs into a lot of issues with the python client and the autoscaler, since they expect bundles to be populated for the pending placement group. To fix it we end up having to modify how bundles get cached and essentially return scheduling_strategy index 0 (the primary strategy) anytime bundles would get called anyway.

The change I tried to implement ended up being pretty expansive, is this something we should try to add in a follow-up PR? Or do you know if I might be missing something and there's an easier way to support the new semantics of the field.

Understood. I think it is a very involved change because the semantic changes.

The current placement group management logic in all places assumes that there is only one set of requirements for each placement group so the requirements and the state tracking are all in the bundles field.

At the same time, with the fallback semantic, while the user facing API remains the bundles will still be used for specifying the highest priority resource requirement, we changed the assumption in GCS placement group management logic and split the original bundles field in PlacementGroupTableData into:

• scheduling_strategy which only contains the scheduling requirements for the bundles
• and bundles which only tracks the bundle id and the corresponding node id for the currently chosen placement group. Only bundle_id and node_id needs to be populated and we might need an additional field to indicate the index of the chosen set of requirements in the scheduling_strategy

Looking into the code, I think on a high level, the semantic changes means:

• The user facing APIs should remain the same in terms of the bundles field and just add fallback_options additional field:
  • semantics in user facing APIs (placement_group.py) shouldn't change
  • semantics in PlacementGroupSpecification, BundleSpecification etc. shouldn't change
• On GCS side, the semantics in GcsPlacementGroup should change to the new semantics. This means:
  (1) The operations related bundle field (GetBundles, GetUnplacedBundles, etc.) should be changed to the semantics of assuming this is the chosen resource requirements. cached_bundle_specs_ should be the chosen
  (2) The placement group scheduling logic needs to consider all the resource requirements in scheduling_strategy when the whole placement group needs to be scheduled.
  (3) The logic to sync the pending placement groups from GCS to autoscaler needs to provide the list of resource requirements

In terms of the PRs, looks like the current PR handles (2) and related changes to update the PlacementGroupTableData and PlacementGroupSpec data model. And if the current didn't change the existing behavior of having one set of requirements in bundles, I think we can add the additional changes that covers (1) and (3) in separate PRs.

Let me know whether the above makes sense or I missed some aspects in handling the bundles semantic changes.

cc: @edoakes for visibility and if you have additional comment.

Makes sense, thanks for outlining it I feel like that made it a lot more clear to implement. I went through and think with af673db I've implemented everything from (2) and part of (1). I changed bundles to remain empty until an active strategy has been selected, but updated GetBundles to return the highest priority strategy when bundles is empty so that it still works with pending PGs. I think a follow-up PR would still be needed so that this field fully follow the new semantics and returns an empty list before a strategy has been chosen (and also updates all downstream consumers to check for

For the autoscaler, I updated GetPendingGangResourceRequests since it reads from the proto to check for scheduling_strategy(0) for PENDING groups, and then in a follow-up PR that addresses the autoscaling side can have it actually consider the full list of fallback options when scheduling.

GCS RPC on every task submission to PG

High Severity

_get_active_strategy_index calls self.wait(timeout_seconds=0) unconditionally on every invocation, which is a GCS RPC. This method is called from both check_placement_group_index and _validate_resource_shape during task/actor submission, adding at least 2 GCS RPCs per submission. Previously, validation used only cached data after the first call. The cached _active_strategy_index_cache is checked after the wait call, so even a cache hit still pays the RPC cost.

 

The options are that we either:

1. Keep the rpc call, since we need to be able to validate the task against the actual scheduling option that is chosen. Since the scheduling option can change if all of the PG bundles get rescheduled, we can't just cache the value once and then use it.
2. Validate against all scheduling options (primary + fallback), rather than the active strategy. This is more permissive and could lead to tasks being accepted that can not be scheduled on the active strategy.
3. Change the scheduler logic again so that we do not consider fallbacks when doing re-scheduling of all bundles. We only try all of the scheduling options (primary + fallbcak) once when initially scheduling, and from that point on the active strategy is locked. This reduces the value of fallback strategies, because if a node is preempted we don't support the PG falling back to a strategy it could schedule on, but simplifies the scheduler and Python logic.

Thanks for the detailed options!

I don't think we should add additional GRPC calls to GCS for all task/actor submission for the validity check. At the same time, I think the reconsidering all the fallback during the whole placement group scheduling is still valuable.

So I think 2 could be the best solution. And at the same time, we need to make sure to output the log message clearly in the infeasible case so that users can be aware of the situation.

Hope that makes sense.