[Open Data] Restore retired SKUs in instance size flexibility and reconcile the ratio scale - #2308
Conversation
The Catalogs API only returns SKUs that can still be purchased, so the instance size flexibility dataset dropped every retired series that active reservations still cover (Av2, D, DS, Dv2, Dv3, Ev3, F, G, H, LS, NC, NV and others). The Optimization Engine joins ISF with kind=inner, so those reservations were silently missing from its reports. - Backfill the retired groups from the ratio file the API replaced: 1,353 -> 2,573 SKUs and 211 -> 318 flexibility groups. Purely additive against v15 -- no row removed and no published ratio changed. - Merge additively instead of overwriting, so a SKU the API stops returning is carried forward rather than dropped, and fail the run if the dataset would shrink. - Enumerate every physical region from the ARM locations API (63) instead of sweeping a hardcoded list of 26, which omitted SKUs that launch in only a handful of regions. regionType is nested under metadata, and the logical groupings it marks are not valid catalog scopes. - Reconcile the ratio scale. The retired file normalized each group so its smallest SKU was 1 while the API reports vCPU counts, and because Azure retires individual sizes the API covers only part of 44 of the 204 shared groups, so a per-SKU merge left single groups holding both units. Backfilled rows were converted onto the API scale once, and Assert-IsfScale now fails the run if a group drifts off it again. - Drop non-positive ratios. The API returns one, and the benefits simulation workbook divides by Ratio. Fixes #2300 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
There was a problem hiding this comment.
🟡 Changes recommended
The generator uses culture-dependent [double]::TryParse(...) for ratios, which can misparse decimal values in non-dot locales and jeopardize dataset correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes gaps in the Instance Size Flexibility (ISF) open-data pipeline by making the generated dataset additive (carrying forward retired-but-still-relevant SKUs), expanding region coverage via ARM locations enumeration, and enforcing invariants around ratio scale and non-positive ratios to prevent downstream consumer errors.
Changes:
- Merge Catalogs API results over the previously published ISF CSV keyed by
ArmSkuName, and fail the run if the dataset would shrink. - Enumerate all physical Azure regions from the ARM locations API when
-Locationis not provided, replacing the hardcoded region list. - Drop non-positive ratios, add scale-consistency validation (
Assert-IsfScale), expand unit tests, and update documentation/changelog to reflect the new behavior.
File summaries
| File | Description |
|---|---|
| src/scripts/Update-InstanceSizeFlexibility.ps1 | Implements additive merge, ARM-based physical region enumeration, ratio filtering, and scale validation logic. |
| src/powershell/Tests/Unit/Update-InstanceSizeFlexibility.Tests.ps1 | Adds unit tests for additive merge behavior, zero-ratio filtering, scale mismatch detection, and region enumeration paths. |
| src/open-data/README.md | Updates dataset behavior documentation (additive merge, region coverage, ratio scale notes, zero-ratio handling). |
| src/open-data/InstanceSizeFlexibility.csv | Backfills and expands the published ISF dataset with retired SKUs and additional groups. |
| docs-mslearn/toolkit/open-data.md | Updates Microsoft Learn documentation to reflect additive dataset behavior, region coverage, and ratio scale handling. |
| docs-mslearn/toolkit/changelog.md | Adds changelog entries describing the fixes and dataset growth. |
| .github/workflows/opendata-instance-size-flexibility.yml | Updates workflow PR body text to align with additive/merge semantics and expected diffs. |
Review details
Suppressed comments (2)
src/scripts/Update-InstanceSizeFlexibility.ps1:337
- This ratio parse also defaults to the current culture. Since the published CSV uses
.for decimals, cultures that expect,can misparse or reject values, which would affect the duplicate-group detection and merge inputs. UseInvariantCulturefor deterministic behavior.
{
$ratio = 0.0
if (-not [double]::TryParse($row.Ratio, [ref]$ratio)) { continue }
if ($ratio -le 0) { continue }
src/scripts/Update-InstanceSizeFlexibility.ps1:346
- Same culture-dependent parsing issue here: using the current culture can mis-handle decimal ratios in the CSV (which are written with
.). UseInvariantCultureto avoid silently changing which rows are retained during the merge.
foreach ($row in $rows)
{
$ratio = 0.0
if (-not [double]::TryParse($row.Ratio, [ref]$ratio)) { continue }
if ($ratio -le 0) { continue }
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
The ratio-scale follow-up flagged in the description is now tracked separately in #2309, so it doesn't hold up this PR. That one is about the Optimization Engine's math; this PR leaves every published ratio unchanged. |
The ratios were described as absolute vCPU counts. That holds for many D/E-series groups by coincidence -- their smallest SKU has 2 vCPUs, so normalizing against it yields a factor of 2 -- but not in general: - 799 of 1,164 parseable VM SKUs match their vCPU count; 365 do not. - Standard_B2as_v2 has 2 vCPUs and a ratio of 16; Standard_B16als_v2 has 16 and a ratio of 113.4. - azure_managed_redis_balanced_b1000 has a ratio of 1248. - 156 of the returned ratios are not integers. What is actually verifiable is that the API leaves ratios unnormalized: only 52 of its 211 groups start at 1, against 432 of 433 in the retired files. That is what the merge and the scale check rely on, so the wording now states it and no longer names a unit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Depends on merge order with #2298, which adds This PR takes the CSV to 2,573 rows, so that generated file has to be rebuilt. It is deliberately not included here: both PRs would otherwise add the same new file and collide. Plan: let #2298 merge first, then merge Note that Open Data CI passing on this PR does not mean the file was regenerated — on Hub KQL is unaffected: |
Ratios were parsed and written with the current culture. On a comma-decimal machine that corrupts the dataset in both directions: - de-DE parses "2.1" as 21, so a ratio is silently ten times too large. - fr-CH fails the parse outright, so the row is dropped. - Export-Csv writes the value back as "2,1", which breaks every consumer: the Optimization Engine's externaldata(... Ratio:double) and the Power BI partitions typed as number. CI runs on ubuntu-latest and is unaffected, but the README documents running the generator by hand, so a contributor in a comma-decimal locale would publish a corrupt file. Both parse sites and the write path now pin InvariantCulture, with a regression test that drives the generator under de-DE and asserts the round trip. Also corrects the SKU count in the changelog: it still said 2,574 from before the zero-ratio row was dropped, against 2,573 actually published. Reported by Copilot review on #2308 (the parse side; the write side turned up while verifying it). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Fixes #2300. The instance size flexibility dataset was missing every VM series that Azure no longer sells, because the Catalogs API only returns SKUs that can still be purchased. Reservations on those SKUs are still active, and the Optimization Engine joins ISF with
kind=inner, so those reservations were silently absent from its reservation reports rather than showing as unmatched.The dataset grows from 1,353 to 2,573 SKUs and 211 to 318 flexibility groups. All 364 rows from the reporter's attachment are present.
Against v15 this is purely additive: 0 rows removed, 0 published ratios changed, schema unchanged at 3 columns. No consumer needs a change.
What changed
Av2,D,DS,Dv2,Dv3,Ev3,F,G,H,LS,NC,NV,SQL on IaaS, …)ArmSkuName, so a SKU the API stops returning is carried forward. The run fails if the dataset would shrink.Assert-IsfScalefails the run if a group drifts off itRegion enumeration
The hardcoded 26-region list omitted SKUs that launch in only a few regions — new GPU and specialty sizes routinely do. It also had to be hand-maintained as Azure adds regions. One detail worth flagging for review:
regionTypeis nested undermetadata, not at the top level, and 46 of the 109 entries are logical groupings (global,unitedstates,europe) that are not valid catalog scopes.Measured cost: 7.9 minutes for 63 regions × 3 reserved types, against the workflow's 60-minute timeout (previously 3–4 minutes for 26).
Ratio scale — please read
The retired ratio file and the Catalogs API express the same proportions in different units:
Standard_DS11_v2Standard_D2s_v4Standard_D64s_v4The retired file normalized each group so its smallest SKU was
1; the API leaves its ratios unnormalized. 432 of 433 retired-file groups start at1; only 52 of 211 API groups do. (The API's unit isn't documented and is not a vCPU count --Standard_B2as_v2has 2 vCPUs and a ratio of 16 -- but the unit isn't what matters here: what matters is that the two sources are scaled differently.)Because Azure retires individual sizes rather than whole families, the API covers only part of many groups — 44 of the 204 groups both sources knew.
Ev3 Seriesis a worked example: the API returns 3 of its 8 SKUs. A per-SKU merge therefore left single groups holding both units, producing non-monotonic ratios (Standard_E2_v3andStandard_E4_v3both at 2). The backfilled rows were converted onto the API scale using the factor derived from the SKUs each group shared with the API, verified against the live API.The consequence to be aware of: the published dataset uses the API scale, which is what v15 already shipped for the rows it contained. The Optimization Engine's
reservations-usage.jsoncomputesTotalReservedQuantity_s * Ratioand names the resultAvgRIsUsedInSmallestRatio— that naming expects the normalized scale. Utilization percentages are unaffected because the factor cancels, but absolute quantities are on the API's scale rather than the normalized one. This is pre-existing since #2222 and is not changed by this PR, since no published ratio changed. Flagging it because it is the natural follow-up decision, and it belongs to whoever owns the AOE math.The 105 fully retired groups have no API counterpart to convert against and necessarily keep the normalized scale. Harmless as long as ratios are compared within their group, which is the only valid use.
Zero ratios
The API returns
azure_redis_cache_isolated_i100with a ratio of0.benefits-simulation.jsondoeslet ISFRatio = toscalar(...)and then divides by it, and its{VMSize}picker is not restricted to VMs — it lists whatever the customer consumes. Non-positive ratios are now dropped, with a unit test asserting the invariant on the published file.Test plan
Invoke-Pester ./src/powershell/Tests/Unit/* ./src/powershell/Tests/Lint/*— 6,017 passed, 0 failed (4 pre-existing skips)externaldata()declarations and both Power BI TMDL partitions; no case-variant group or SKU names (KQLsummarize byis case-sensitive); no commas or quotes in group names;x_SkuTypestays unique for the Power BI relationship; workbook pickers are driven by customer consumption ∩ ISF, so the extra rows do not pollute them; hubs do not consume the fileEv3 Seriesmixed-scale case and all four region-enumeration pathsNotes for reviewers
LastSeencolumn, deliberately. The additive merge means a SKU can persist after the API stops returning it, which invites a column recording when it was last confirmed. It would cost the published schema: 21externaldata(ISFGroup:string, ArmSKUName:string, Ratio:double)declarations across the three workbooks pin three positional columns, both Power BI partitions pinColumns = 3, and external consumers of a published open-data file would break. The information is already available for free -- the weekly workflow commits this file, sogit log -- src/open-data/InstanceSizeFlexibility.csvgives the last-confirmed date per row without touching the contract.Set-AzContextAutosaveSettingno longer exists in Az.Accounts 3.x, which matters if you reproduce the run locally with a context-isolation helper;Disable-AzContextAutosave -Scope Processis the current equivalent.🤖 Generated with Claude Code