Repository navigation
ux(reshape): composite score ranking for cross-family alternatives (closes #153) - #836
Conversation
) Replace the ascending-by-EffectiveMonthlyCost sort in fillAlternativesFromRecs with a compositeScore function that blends five weighted signals: cost (60%): dominant term, cheaper offerings score higher familyGen (15%): same-prefix generation jump (m5->m6i) bonus arch (10%): cross-architecture (x86->ARM) penalty confidence (10%): CE savings+count confidence heuristic nfProximity (5%): normalized-capacity proximity bonus Absent signals return 0.5 neutral to avoid penalising offerings for missing data (per nullable-not-zero guideline). Add SavingsAbs *float64 and RecommendationCount int fields to OfferingOption; populate them in recommendationToOffering from RecommendationRecord.Savings and .Count. Add 25 new tests: unit tests for each score component helper plus an integration snapshot that asserts m6i > m6g > c5 ordering.
cristim
added
triaged
|
@coderabbitai review |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements composite-score ranking for EC2 instance reshaping alternatives. The ChangesComposite Score Ranking for Cross-Family Alternatives
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai review |
Rate Limit Exceeded
|
… tie-break (refs #153) isARMFamily used strings.HasPrefix(suffix, "g") which produced false positives for the Intel Dense Storage/Network families is4gen and im4gn: both have a suffix that starts with "g" after stripping letters+digits, but they are x86-only processors. Fix: exclude the "im" and "is" prefixes explicitly (reusing familyPrefix), then require an exact match against the known Graviton suffix set {"g", "gd", "gn"} instead of a prefix check. The alternatives sort.Slice had a two-key comparator (compositeScore desc, EMC asc); two offerings with equal score and equal EMC could order non-deterministically. Fix: add InstanceType as the final lexicographic tie-breaker. Tests: extend TestCompositeScore_ARMDetection with is4gen, im4gn (x86 -> false), g4dn, g5 (NVIDIA GPU -> false). Add TestFillAlternativesFromRecs_DeterministicOrder: delivers two equal-score equal-EMC offerings in reverse-lexical order and asserts lexicographic output -- fails pre-fix, passes post-fix.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/exchange/reshape_crossfamily_test.go (1)
455-459: ⚡ Quick winMisleading test name: "TermMismatch" should be "CrossFamily".
The test name
TestCompositeScore_SameGenOutranksTermMismatchsuggests it's testing RI term duration mismatches (1y vs 3y), but the test actually validates that same-family-prefix alternatives (m5→m6i) outrank cross-family alternatives (m5→r5). In the RI context, "term" conventionally refers to commitment duration, not family lineage.📝 Suggested name fix
-// TestCompositeScore_SameGenOutranksTermMismatch asserts that a +// TestCompositeScore_SameGenOutranksCrossFamily asserts that a // near-perfect alternative (same family prefix, exact NF, high confidence) // outranks an alternative with identical savings but no family match and // no confidence signal. -func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { +func TestCompositeScore_SameGenOutranksCrossFamily(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/exchange/reshape_crossfamily_test.go` around lines 455 - 459, Rename the misleading test TestCompositeScore_SameGenOutranksTermMismatch to reflect cross-family behavior (e.g., TestCompositeScore_SameGenOutranksCrossFamily) and update any references to this function name; specifically change the test function name declaration and any calls or mentions (TestCompositeScore_SameGenOutranksTermMismatch) and the comment header to match the new name so it clearly documents that it validates same-family-prefix alternatives outranking cross-family alternatives.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/exchange/reshape.go`:
- Around line 308-334: The confidenceComponent function currently coerces
RecommendationCount == 0 to 1 which can incorrectly penalize offers when the
count is actually missing; change the logic in confidenceComponent (which takes
an OfferingOption) so that if off.RecommendationCount == 0 you treat it as
"missing" and return 0.5 (neutral) instead of coercing to 1 — otherwise use the
actual count for the existing savings thresholds (savings >= 200 && count >= 3
-> 1.0, savings >= 50 -> 0.5, else 0.0).
---
Nitpick comments:
In `@pkg/exchange/reshape_crossfamily_test.go`:
- Around line 455-459: Rename the misleading test
TestCompositeScore_SameGenOutranksTermMismatch to reflect cross-family behavior
(e.g., TestCompositeScore_SameGenOutranksCrossFamily) and update any references
to this function name; specifically change the test function name declaration
and any calls or mentions (TestCompositeScore_SameGenOutranksTermMismatch) and
the comment header to match the new name so it clearly documents that it
validates same-family-prefix alternatives outranking cross-family alternatives.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ffe1ccf-c756-4c65-ae26-6fe871998513
📒 Files selected for processing (3)
internal/api/exchange_lookup.gopkg/exchange/reshape.gopkg/exchange/reshape_crossfamily_test.go
| // confidenceComponent maps SavingsAbs + RecommendationCount to a [0,1] | ||
| // confidence score using the same thresholds as the API layer: | ||
| // | ||
| // high ($200+ savings, 3+ instances) -> 1.0 | ||
| // medium ($50+ savings) -> 0.5 | ||
| // low (otherwise) -> 0.0 | ||
| // | ||
| // When SavingsAbs is nil (not supplied) the component returns 0.5 (neutral) | ||
| // so that an absent field doesn't tilt the ranking. | ||
| func confidenceComponent(off OfferingOption) float64 { | ||
| if off.SavingsAbs == nil { | ||
| return 0.5 | ||
| } | ||
| savings := *off.SavingsAbs | ||
| count := off.RecommendationCount | ||
| if count < 1 { | ||
| count = 1 | ||
| } | ||
| switch { | ||
| case savings >= 200 && count >= 3: | ||
| return 1.0 | ||
| case savings >= 50: | ||
| return 0.5 | ||
| default: | ||
| return 0.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle missing RecommendationCount as neutral in confidence scoring.
At Line 323, missing/zero RecommendationCount is coerced to 1, which can still produce a low-confidence penalty when count is actually absent. That conflicts with the neutral-missing contract documented at Lines 94-95.
Proposed fix
func confidenceComponent(off OfferingOption) float64 {
- if off.SavingsAbs == nil {
+ if off.SavingsAbs == nil || off.RecommendationCount <= 0 {
return 0.5
}
savings := *off.SavingsAbs
count := off.RecommendationCount
- if count < 1 {
- count = 1
- }
switch {
case savings >= 200 && count >= 3:
return 1.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // confidenceComponent maps SavingsAbs + RecommendationCount to a [0,1] | |
| // confidence score using the same thresholds as the API layer: | |
| // | |
| // high ($200+ savings, 3+ instances) -> 1.0 | |
| // medium ($50+ savings) -> 0.5 | |
| // low (otherwise) -> 0.0 | |
| // | |
| // When SavingsAbs is nil (not supplied) the component returns 0.5 (neutral) | |
| // so that an absent field doesn't tilt the ranking. | |
| func confidenceComponent(off OfferingOption) float64 { | |
| if off.SavingsAbs == nil { | |
| return 0.5 | |
| } | |
| savings := *off.SavingsAbs | |
| count := off.RecommendationCount | |
| if count < 1 { | |
| count = 1 | |
| } | |
| switch { | |
| case savings >= 200 && count >= 3: | |
| return 1.0 | |
| case savings >= 50: | |
| return 0.5 | |
| default: | |
| return 0.0 | |
| } | |
| } | |
| // confidenceComponent maps SavingsAbs + RecommendationCount to a [0,1] | |
| // confidence score using the same thresholds as the API layer: | |
| // | |
| // high ($200+ savings, 3+ instances) -> 1.0 | |
| // medium ($50+ savings) -> 0.5 | |
| // low (otherwise) -> 0.0 | |
| // | |
| // When SavingsAbs is nil (not supplied) the component returns 0.5 (neutral) | |
| // so that an absent field doesn't tilt the ranking. | |
| func confidenceComponent(off OfferingOption) float64 { | |
| if off.SavingsAbs == nil || off.RecommendationCount <= 0 { | |
| return 0.5 | |
| } | |
| savings := *off.SavingsAbs | |
| count := off.RecommendationCount | |
| switch { | |
| case savings >= 200 && count >= 3: | |
| return 1.0 | |
| case savings >= 50: | |
| return 0.5 | |
| default: | |
| return 0.0 | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/exchange/reshape.go` around lines 308 - 334, The confidenceComponent
function currently coerces RecommendationCount == 0 to 1 which can incorrectly
penalize offers when the count is actually missing; change the logic in
confidenceComponent (which takes an OfferingOption) so that if
off.RecommendationCount == 0 you treat it as "missing" and return 0.5 (neutral)
instead of coercing to 1 — otherwise use the actual count for the existing
savings thresholds (savings >= 200 && count >= 3 -> 1.0, savings >= 50 -> 0.5,
else 0.0).
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
EffectiveMonthlyCostsort infillAlternativesFromRecswith acompositeScorefunction that blends five weighted signals (cost 60%, family-gen 15%, arch 10%, CE confidence 10%, NF proximity 5%).SavingsAbs *float64andRecommendationCount inttoOfferingOption; populate them inrecommendationToOfferingfromRecommendationRecordso the confidence signal flows through without the exchange package importing internal config types.Signal weights (named consts in
reshape.go)scoreWeightCostscoreWeightFamilyGenscoreWeightArchscoreWeightConfidencescoreWeightNFProximityTest plan
TestCompositeScore_SameGenOutranksTermMismatch- near-perfect m6i outranks cheaper r5 with no family matchTestCompositeScore_SameArchOutranksCrossArch- x86->x86 outranks x86->ARM when family-gen is equalTestCompositeScore_HighConfidenceOutranksLow- high-confidence CE rec beats low-confidence at equal costTestCompositeScore_AbsentSavingsIsNeutral- nil SavingsAbs treated as neutral (0.5), not low-confidence (0.0)TestCompositeScore_FamilyPrefixDetection- familyPrefix strips generation digits correctlyTestCompositeScore_ARMDetection- isARMFamily classifies Graviton vs x86/AMD familiesTestAnalyzeReshapingWithRecs_CompositeScoreOrdersAlternatives- snapshot integration: m6i > m6g > c5Closes #153
Summary by CodeRabbit