Skip to content

Conversation

mrDzurb
Copy link
Member

@mrDzurb mrDzurb commented Aug 29, 2025

Description

This PR updates the GPU shapes index file to:

  • Add new GPU shapes
  • Include additional CPU parameters

Motivation

Keeping the GPU shapes index up to date ensures accurate configuration and compatibility across deployments. Adding CPU parameters provides more complete resource specifications for better validation and deployment planning.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Aug 29, 2025
@mrDzurb mrDzurb requested review from elizjo, lu-ohai and Aryanag2 August 29, 2025 23:36
darenr
darenr previously approved these changes Aug 29, 2025
Copy link
Member

@darenr darenr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some potential issues found in the pull request changes:

  1. ads/aqua/common/entities.py:

    • The ComputeRank and GPUSpecs models now use Optional[int] for fields that were previously just int. If any downstream code assumes these fields are always set (non-None), it could cause runtime errors or unexpected behavior.

    • The validator method in ComputeShapeSummary was renamed from set_gpu_specs to populate_gpu_specs, which is good for clarity, but make sure all references are updated elsewhere.

    • There is a code comment referencing extracting GPU count with regex for shape names like "VM.GPU3.2", but the regex used is r"\.(\d+)$"—this will not match "VM.GPU3.2" but will match "VM.GPU.A10.2" (dot before number at end). If you expect both formats, this may miss certain shapes.

    • Logging inside the validator uses logger.debug, but if logger is not defined or imported, this will cause a NameError.

  2. ads/aqua/resources/gpu_shapes_index.json:

    • The JSON structure now includes new top-level items like "BM.GPU.B200.8" and "BM.GPU.GB200.4" outside the "shapes" object. This breaks the previous schema, where all shapes were inside the "shapes" key. Downstream code expecting all shapes under "shapes" will miss the new entries.

    • The new shapes and CPU fields are comprehensive, but check that clients or code loading this file are updated to handle the new structure and that it won’t break existing deployments.

Summary:

  • There are potential compatibility issues with both the Python and JSON changes.
  • Make sure all code that reads or validates this data is aware of the new schema and field behaviors.
  • Double-check shape name parsing and logger definitions.

Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.29%

@mrDzurb mrDzurb requested a review from darenr September 3, 2025 03:11
Copy link

github-actions bot commented Sep 3, 2025

📌 Cov diff with main:

Coverage-100%

📌 Overall coverage:

Coverage-58.33%

)

@model_validator(mode="after")
@classmethod
def set_gpu_specs(cls, model: "ComputeShapeSummary") -> "ComputeShapeSummary":
def populate_gpu_specs(cls, model: "ComputeShapeSummary") -> "ComputeShapeSummary":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm assuming this name change would not affect elsewhere since I didn't come across any references/usage within ads.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this is just a validator, will not affect the final logic.

@mrDzurb mrDzurb enabled auto-merge (squash) September 4, 2025 00:52
@mrDzurb mrDzurb merged commit f8b2f5f into main Sep 4, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants