[Subnet Prioritization] Support capacity-optimized-prioritized and prioritized Allocation Strategy #671
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…leet call Signed-off-by: Hanxuan Zhang <[email protected]>
Signed-off-by: Hanxuan Zhang <[email protected]>
…rams launching with EnableSingleAvailabilityZone and prioritized|capacity-optimized-prioritized AllocationStrategy Signed-off-by: Hanxuan Zhang <[email protected]>
Signed-off-by: Hanxuan Zhang <[email protected]>
…ity-optimized-prioritized when using EnableSingleAvailabilityZone Signed-off-by: Hanxuan Zhang <[email protected]>
himani2411
reviewed
Jun 18, 2025
src/slurm_plugin/fleet_manager.py
Outdated
| # set SingleAvailabilityZone to False | ||
| "SingleAvailabilityZone": ( | ||
| self._compute_resource_config["Networking"]["SingleAvailabilityZone"] | ||
| if self._compute_resource_config["Networking"]["SingleAvailabilityZone"] is not None |
There was a problem hiding this comment.
I would suggest that you add a condition here where we check if the allocation strategy is the one we want and then apply SingleAvailibilityZone. We don't want to use this parameter if we use lowest-price AllocationStrategy
src/slurm_plugin/fleet_manager.py
Outdated
| for instance_type in self._compute_resource_config["Instances"]: | ||
| subnet_ids = self._compute_resource_config["Networking"]["SubnetIds"] | ||
| for subnet_id in subnet_ids: | ||
| overrides.update({"InstanceType": instance_type["InstanceType"], "SubnetId": subnet_id}) | ||
| if ( | ||
| self._compute_resource_config.get("AllocationStrategy") == "prioritized" |
There was a problem hiding this comment.
We should also check if the CapacityType is the one that we want along with allocation strategy, the reason we should add that even if that's already taken care in CLI validator is because customer can suppress or ignore and move forward.
CHANGELOG.md
Outdated
| - There were no changes for this version. | ||
| - Support prioritized|capacity-optimized-prioritized Allocation Strategy and EnableSingleAvailabilityZone |
src/slurm_plugin/fleet_manager.py
Outdated
Comment on lines
331
to
332
| self._compute_resource_config["Networking"]["SingleAvailabilityZone"] | ||
| if self._compute_resource_config["Networking"]["SingleAvailabilityZone"] is not None |
There was a problem hiding this comment.
Are you sure self._compute_resource_config always has "Networking" section?
himani2411
reviewed
Jun 20, 2025
src/slurm_plugin/fleet_manager.py
Outdated
| self._compute_resource_config["Networking"]["SingleAvailabilityZone"] | ||
| if self._compute_resource_config["Networking"]["SingleAvailabilityZone"] is not None | ||
| and ( | ||
| self._compute_resource_config.get("AllocationStrategy") == "prioritized" |
There was a problem hiding this comment.
Nitpick: Can we check these conditions using a function as we keep checking this above an here
Signed-off-by: Hanxuan Zhang <[email protected]>
Signed-off-by: Hanxuan Zhang <[email protected]>
Signed-off-by: Hanxuan Zhang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Tests
References
Checklist
developadd the branch name as prefix in the PR title (e.g.[release-3.6]).Please review the guidelines for contributing and Pull Request Instructions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.