Skip to content

fix: AcceleratorType is nil for MI300X sizes - #37

Open
Jose Blanquicet (blanquicet) wants to merge 1 commit into
Azure:mainfrom
blanquicet:jose/fix-accelerator-type-mi300x
Open

fix: AcceleratorType is nil for MI300X sizes#37
Jose Blanquicet (blanquicet) wants to merge 1 commit into
Azure:mainfrom
blanquicet:jose/fix-accelerator-type-mi300x

Conversation

@blanquicet

Copy link
Copy Markdown
Member

The accelerator group has two options:

(xl_[A-Z]+[0-9]+[A-Z]*|[A-Z]+[0-9]+)

The first one allows letters after the digits (the trailing [A-Z]*), but only if the name starts with xl_. The second one has no xl_, but does not allow letters after the digits either.

MI300X needs the trailing letters and has no xl_ prefix, so it matched neither, and ND96isr_MI300X_v5 and ND96is_MI300X_v5 ended up with a nil AcceleratorType.

This makes the xl_ part optional inside the group instead of keeping two separate options:

((?:xl_)?[A-Z]+[0-9]+[A-Z]*)

Both MI300X sizes now get AcceleratorType set. I checked the 1385 sizes in testdata.SKUData: all of them still parse, and these two are the only ones that changed.

Added both MI300X sizes to testCases, plus A100, H100, H200 and A10 sizes, which had no cases there before. H100 and H200 share the same Series (NDisr_v5), so AcceleratorType is what tells them apart.

Same change in v1 and v2.

The accelerator group only allowed letters after the digits when the name
also had the "xl_" prefix, so "MI300X" matched neither option and
ND96isr_MI300X_v5 and ND96is_MI300X_v5 ended up with a nil AcceleratorType.

Made the "xl_" part optional inside the group instead of keeping two
separate options. Both MI300X sizes now get AcceleratorType set, and the
1385 sizes in testdata still parse the same way.

Added both MI300X sizes to testCases, plus A100, H100, H200 and A10 sizes,
which had no cases there before. H100 and H200 share the same Series
(NDisr_v5), so AcceleratorType is what tells them apart.

Same change in v1 and v2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant