-
Notifications
You must be signed in to change notification settings - Fork 52
[WIP][AQUA] GPU Shape Recommendation #1221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
2f54f8b
to
26e08a2
Compare
}, | ||
"VM.GPU.A10.1": { | ||
"gpu_count": 1, | ||
"gpu_memory_in_gbs": 24, | ||
"gpu_type": "A10" | ||
"gpu_type": "A10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add FP8 for the A10 shapes as well.
ads/aqua/common/utils.py
Outdated
@@ -1287,6 +1287,7 @@ def load_gpu_shapes_index( | |||
|
|||
# Merge: remote shapes override local | |||
local_shapes = local_data.get("shapes", {}) | |||
remote_data = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
ads/aqua/extension/__init__.py
Outdated
@@ -13,6 +13,7 @@ | |||
from ads.aqua.extension.evaluation_handler import __handlers__ as __eval_handlers__ | |||
from ads.aqua.extension.finetune_handler import __handlers__ as __finetune_handlers__ | |||
from ads.aqua.extension.model_handler import __handlers__ as __model_handlers__ | |||
from ads.aqua.extension.recommend_handler import __handlers__ as __gpu_handlers__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can name it as __shape_handler
?
Detects quantization bit-width as a string (e.g., '4bit', '8bit') from Hugging Face config dict. | ||
""" | ||
if raw.get("load_in_8bit"): | ||
return "8bit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to move this into constants.
If model is un-quantized, uses the weight size. | ||
If model is pre-quantized, uses the quantization level. | ||
""" | ||
key = (self.quantization or self.weight_dtype or "float32").lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move "float32" to constants
""" | ||
vals = [] | ||
curr = min_len | ||
max_seq_len = 16384 if not self.max_seq_len else self.max_seq_len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move the numbers like 16384 to constants and add some description there
""" | ||
|
||
@handle_exceptions | ||
def post(self, *args, **kwargs): # noqa: ARG002 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to move this handler to deployment_handler.py
under the AquaDeploymentHandler
class?
We already have a list_shape
method there, so I suggest adding a new method called list_recommended_shape
.
The endpoint path could be /aqua/deployments/recommended_shapes
.
The main reason for this change is that we’ll be implementing similar logic for service models as well. Having a unified handler will allow us to return the recommended list of shapes for both service and custom models in a consistent way.
ads/aqua/shaperecommend/recommend.py
Outdated
ShapeRecommendationReport, | ||
ShapeReport, | ||
) | ||
from ads.config import COMPARTMENT_OCID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this const variable is not used?
ads/aqua/shaperecommend/recommend.py
Outdated
return self.rich_diff_table(shape_recommend_report) | ||
|
||
@staticmethod | ||
def validate_model_ocid(ocid: str) -> DataScienceModel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not planning to use this method outside of the class, it would be better to make it as private.
def _validate_model_ocid
. Check for the others.
ads/aqua/shaperecommend/recommend.py
Outdated
from ads.model.datascience_model import DataScienceModel | ||
|
||
|
||
class AquaRecommendApp(AquaApp): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Maybe we can name it as AquaShapeRecommendApp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, the shape recommendation feature falls under the broader Model Deployment functionality. Instead of creating a new App
class, I suggest we implement the business logic in a regular helper class and then integrate it into the existing AquaDeploymentApp
.
Since AquaDeploymentApp
already includes a list_shapes
method, we can add a new method called list_recommended_shapes
to maintain backward compatibility.
Looking ahead, if we need similar logic for other modules like fine-tuning or evaluation, we can follow the same pattern and add the corresponding methods in those apps as well.
As for naming, we could rename the current AquaRecommendApp
to AquaShapeRecommend
. I don't think we need to inherit from AquaApp
in this case, since it's primarily a utility class focused on recommendation logic.
ads/aqua/shaperecommend/recommend.py
Outdated
Use `ads aqua recommend which_gpu --help` to get more details on available parameters. | ||
""" | ||
|
||
def which_gpu(self, **kwargs) -> ShapeRecommendationReport: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to make it a bit more generic - which_shape
?
ads/aqua/shaperecommend/recommend.py
Outdated
return recommendations | ||
|
||
@staticmethod | ||
def rich_diff_table(shape_report: ShapeRecommendationReport) -> Table: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can make it private?
ads/aqua/shaperecommend/recommend.py
Outdated
from ads.model.datascience_model import DataScienceModel | ||
|
||
|
||
class AquaRecommendApp(AquaApp): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, the shape recommendation feature falls under the broader Model Deployment functionality. Instead of creating a new App
class, I suggest we implement the business logic in a regular helper class and then integrate it into the existing AquaDeploymentApp
.
Since AquaDeploymentApp
already includes a list_shapes
method, we can add a new method called list_recommended_shapes
to maintain backward compatibility.
Looking ahead, if we need similar logic for other modules like fine-tuning or evaluation, we can follow the same pattern and add the corresponding methods in those apps as well.
As for naming, we could rename the current AquaRecommendApp
to AquaShapeRecommend
. I don't think we need to inherit from AquaApp
in this case, since it's primarily a utility class focused on recommendation logic.
ads/aqua/shaperecommend/recommend.py
Outdated
ValueError | ||
If the file cannot be opened, parsed, or the 'shapes' key is missing. | ||
""" | ||
user_shapes = AquaDeploymentApp().list_shapes(compartment_id=compartment_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid calling AquaDeploymentApp().list_shapes
, I think it would be cleaner to add a shapes
method directly to the OCIDataScienceModelDeployment
class.
For example:
class OCIDataScienceModelDeployment(
OCIDataScienceMixin,
oci.data_science.models.ModelDeployment,
):
@classmethod
def shapes(
cls,
compartment_id: str = None,
**kwargs,
) -> List[oci.data_science.models.ModelDeploymentShapeSummary]:
return oci.pagination.list_call_get_all_results(
cls().client.list_model_deployment_shapes,
compartment_id or COMPARTMENT_ID,
**kwargs
).data
This makes the logic more self-contained within the deployment model and avoids relying on external app instances just to retrieve shape info.
The usage could be:
user_shapes = OCIDataScienceModelDeployment.shapes(compartment_id=compartment_id)
ads/aqua/shaperecommend/recommend.py
Outdated
if name in set_user_shapes: | ||
compute_shape = set_user_shapes.get(name) | ||
compute_shape.available = True | ||
compute_shape.shape_series = "GPU" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The oci.data_science.models.ModelDeploymentShapeSummary
already contains the def shape_series(self, shape_series):
. Maybe we can use it?
|
||
valid_shapes = [] | ||
# only loops through GPU shapes, update later to include CPU shapes | ||
for name, spec in gpu_shapes_metadata.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the future- we would need a list of CPU shapes available (similar to load_gpu_shapes_index()). We need this since we make recommendations based on what shapes are possible, not what shapes are currently available
@@ -57,6 +57,16 @@ def get(self, id: Union[str, List[str]] = None): | |||
return self.get_deployment_config( | |||
model_id=id.split(",") if "," in id else id | |||
) | |||
elif paths.startswith("aqua/deployments/recommend_shapes"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: /recommended_shapes
would be better.
|
||
compartment_id = self.get_argument("compartment_id", default=COMPARTMENT_OCID) | ||
|
||
generate_table = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to a different output format? If so, the handler should always return a JSON response regardless of the format.
@@ -57,6 +57,16 @@ def get(self, id: Union[str, List[str]] = None): | |||
return self.get_deployment_config( | |||
model_id=id.split(",") if "," in id else id | |||
) | |||
elif paths.startswith("aqua/deployments/recommend_shapes"): | |||
id = id or self.get_argument("model_id", default=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? Looks like in case of the aqua/deployments/config
we don't check the "model_id"?
@@ -11,3 +11,5 @@ | |||
|
|||
DEFAULT_WAIT_TIME = 12000 | |||
DEFAULT_POLL_INTERVAL = 10 | |||
|
|||
SHAPE_MAP = {"NVIDIA_GPU": "GPU"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, here the full list of supported series.
SHAPE_SERIES_AMD_ROME = "AMD_ROME"
SHAPE_SERIES_INTEL_SKYLAKE = "INTEL_SKYLAKE"
SHAPE_SERIES_NVIDIA_GPU = "NVIDIA_GPU"
SHAPE_SERIES_GENERIC = "GENERIC"
SHAPE_SERIES_LEGACY = "LEGACY"
SHAPE_SERIES_ARM = "ARM"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did research this- the issue is that I'm not 100% sure if these map to GPU/CPU types. For now, I will map all to CPU types except for NVIDIA_GPU since the AMD GPU shape (MX300) did not have the AMD_ROME for the shape_series parameter. I also did not see any of these series (except for NVIDIA_GPU) when we queried for GPU only shapes.
Must be used within a properly configured and authenticated OCI environment. | ||
""" | ||
|
||
def which_shapes(self, **kwargs) -> Union[ShapeRecommendationReport, Table]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of kwargs, we can accept RequestRecommend
?
def which_shapes(self, request: RequestRecommend)
I think that how it was done initially?
ocid : str | ||
OCID of the model to recommend feasible compute shapes. | ||
available_shapes : List[ComputeShapeSummary] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to prepare the available shapes in advance and pass them to the recommender, I think it would make more sense to move this parameter into the constructor:
class AquaShapeRecommend(BaseModel):
available_shapes: List[ComputeShapeSummary] = Field(
..., description="List of available shapes in OCI."
)
This way, we can decouple the shape fetching logic from the recommender logic and keep the design cleaner.
Alternatively, as previously discussed, we could define a method like def available_shapes()
directly within the AquaShapeRecommend
class. This method can internally use the OciDataScienceModelDeployment.shapes()
function to retrieve the initial list of shapes. We can then move the valid_compute_shapes
mehtod form deployment.py
there.
My vote would be for the second approach.
Wrote an additional POST API and aqua command for recommending GPU shapes for a particular model
Returns

Returns
Status: business logic works, API works, unit tests finished, rich diff CLI table finished
