[Core]acceleratorclass cheapest selection policy logic update#547
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the selectCheapest logic to group and prioritize accelerator costs by type (hourly, token-based, and tier) rather than using a single global priority. It also cleans up associated helper functions and tests. A significant concern was raised regarding the removal of documentation for empty AcceleratorClasses lists; there appears to be a logic discrepancy where some parts of the codebase treat an empty list as 'supporting all accelerators' while others treat it as 'supporting none'. This inconsistency should be resolved to ensure predictable behavior.
310351b to
f759002
Compare
f759002 to
7e35190
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the selectCheapest logic to prioritize cost types in groups (hourly, token, and tier) and updates SupportsAcceleratorClass to return false when requirements are undefined. It also modifies candidatesFromNames to preserve the order of names during deduplication. Feedback suggests refactoring the selectCheapest loop into a single pass to improve efficiency and code structure.
| // Group 1: Hourly pricing (SpotPerHour or PerHour — same unit $/hour) | ||
| var cheapest *v1beta1.AcceleratorClass | ||
| var cheapestCost *resource.Quantity | ||
| for i := range withCost { | ||
| c := &withCost[i] | ||
| var cost *resource.Quantity | ||
| if c.Spec.Cost.SpotPerHour != nil { | ||
| cost = c.Spec.Cost.SpotPerHour | ||
| } else if c.Spec.Cost.PerHour != nil { | ||
| cost = c.Spec.Cost.PerHour | ||
| } | ||
| if cost == nil { | ||
| continue | ||
| } | ||
| if cheapest == nil || compareCosts(cost, cheapestCost) < 0 { | ||
| cheapest = c | ||
| cheapestCost = cost | ||
| } | ||
| } | ||
| if cheapest != nil { | ||
| logger.Info("Cheapest selected", "name", cheapest.Name, "costType", "hourly") | ||
| return &cheapest.Name | ||
| } | ||
|
|
||
| logger.Info("Cheapest selected", "name", cheapest.candidate.Name, "costType", cheapest.costValue) | ||
| return &cheapest.candidate.Name | ||
| // Group 2: Token-based pricing ($/million tokens) | ||
| for i := range withCost { | ||
| c := &withCost[i] | ||
| cost := c.Spec.Cost.PerMillionTokens | ||
| if cost == nil { | ||
| continue | ||
| } | ||
| if cheapest == nil || compareCosts(cost, cheapestCost) < 0 { | ||
| cheapest = c | ||
| cheapestCost = cost | ||
| } | ||
| } | ||
| if cheapest != nil { | ||
| logger.Info("Cheapest selected", "name", cheapest.Name, "costType", "per-million-tokens") | ||
| return &cheapest.Name | ||
| } | ||
|
|
||
| // Group 3: Tier fallback (low=1, medium=2, high=3) | ||
| var cheapestTier int64 | ||
| for i := range withCost { | ||
| c := &withCost[i] | ||
| if c.Spec.Cost.Tier == "" { | ||
| continue | ||
| } | ||
| tierVal := tierToNumeric(c.Spec.Cost.Tier) | ||
| if cheapest == nil || tierVal < cheapestTier { | ||
| cheapest = c | ||
| cheapestTier = tierVal | ||
| } | ||
| } | ||
| if cheapest != nil { | ||
| logger.Info("Cheapest selected", "name", cheapest.Name, "costType", "tier") | ||
| return &cheapest.Name | ||
| } |
There was a problem hiding this comment.
The current implementation iterates over the withCost slice three times, once for each cost group (hourly, token, tier). This is slightly inefficient and leads to some code repetition.
Consider refactoring this to first categorize candidates into their respective cost groups in a single pass, and then find the cheapest within each group according to priority. This would improve efficiency by reducing passes over the data and could make the logic more modular.
For example:
// 1. Single pass to group candidates
hourlyCandidates := []v1beta1.AcceleratorClass{}
tokenCandidates := []v1beta1.AcceleratorClass{}
tierCandidates := []v1beta1.AcceleratorClass{}
for _, c := range withCost {
// categorize and append to appropriate list based on cost type
}
// 2. Process groups in order of priority
if len(hourlyCandidates) > 0 {
// find and return cheapest from hourlyCandidates
}
if len(tokenCandidates) > 0 {
// find and return cheapest from tokenCandidates
}
if len(tierCandidates) > 0 {
// find and return cheapest from tierCandidates
}
What this PR does
Fix cheapest accelerator selection to compare costs within the same unit group instead of mixing different pricing types (e.g., "$/hour" vs "$/million-tokens")
Costs are now compared in priority order: hourly (spot + on-demand) → per-million-tokens → tier
Compare candidates in unit groups, not individual cost types:
SpotPerHourif available, elsePerHour. Compare all candidates that have either. Pick cheapest.SpotPerHourandPerHourare both $/hour, so they CAN be compared directly. Spot is just a discounted hourly rate — if B's PerHour < A's SpotPerHour, B is genuinely cheaper.PerMillionTokens. Pick cheapest.Why we need it
The previous selectCheapest used getCandidateCost to extract a single cost per candidate with a fixed priority (spot > on-demand > token > tier), then compared all candidates against each other. This caused two issues:
Fixes #518
How to test
Checklist
make testpasses locally