Skip to content

Conversation

Aryanag2
Copy link
Member

@Aryanag2 Aryanag2 commented Sep 9, 2025

The Aqua Shape Recommender now accepts Hugging Face model IDs in addition to OCI model OCIDs and makes the compartment_id parameter optional.

Key changes:

  • The recommender now checks if a Hugging Face model ID is provided and fetches its configuration accordingly.
  • The compartment_id parameter is now optional. If not provided, it will be sourced from environment variables. An error is raised if no compartment ID can be found.

ensure HF_TOKEN and NB_SESSION_COMPARTMENT_OCID or PROJECT_COMPARTMENT_OCID are set as environment variables
aqua recommend --model-id "distilbert-base-uncased"

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 9, 2025
@Aryanag2 Aryanag2 requested a review from elizjo September 9, 2025 00:07
Copy link

github-actions bot commented Sep 9, 2025

📌 Cov diff with main:

Coverage-63%

📌 Overall coverage:

Coverage-58.33%

Copy link

github-actions bot commented Sep 9, 2025

📌 Cov diff with main:

Coverage-62%

📌 Overall coverage:

Coverage-58.33%

Copy link

📌 Cov diff with main:

Coverage-62%

📌 Overall coverage:

Coverage-58.33%

Copy link

📌 Cov diff with main:

Coverage-65%

📌 Overall coverage:

Coverage-58.39%

@mrDzurb mrDzurb changed the title Add Hugging Face model support to Shape Recommender [AQUA] Add Hugging Face model support to Shape Recommender Sep 10, 2025
Copy link

📌 Cov diff with main:

Coverage-65%

📌 Overall coverage:

Coverage-58.39%

Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.25%

1 similar comment
Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.25%

Copy link

📌 Cov diff with main:

Coverage-41%

📌 Overall coverage:

Coverage-58.36%

Copy link

📌 Cov diff with main:

Coverage-41%

📌 Overall coverage:

Coverage-58.37%

Copy link

📌 Cov diff with main:

Coverage-41%

📌 Overall coverage:

Coverage-58.37%

"UNKNOWN_ENUM_VALUE": "N/A",
}

HUGGINGFACE_CONFIG_URL = "https://huggingface.co/{model_id}/resolve/main/config.json"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it is not used anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

# Copyright (c) 2025 Oracle and/or its affiliates.
# Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/

#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to duplicate this?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

logger.info(
f"'{model_id}' is not an OCID, treating as a Hugging Face model ID."
)
# if not compartment_id:
Copy link
Member

Choose a reason for hiding this comment

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

Let’s copy the commented-out code somewhere else to keep the main code clean.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

from typing import List, Union
import os
import json
from typing import List, Union, Optional, Dict, Any, Tuple
Copy link
Member

Choose a reason for hiding this comment

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

Looks like type - Any is not used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

done


import shutil
from typing import List, Union
import os
Copy link
Member

Choose a reason for hiding this comment

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

With your formatter, could you sort the imports?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return shape_recommendation_report

def valid_compute_shapes(self, compartment_id: str) -> List["ComputeShapeSummary"]:
def _get_model_config_and_name(
Copy link
Member

Choose a reason for hiding this comment

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

NIT: It would be more clean if we had return at the end:

    if is_valid_ocid(model_id):
        logger.info(f"Detected OCID: Fetching OCI model config for '{model_id}'.")
        ds_model = self._validate_model_ocid(model_id)
        config = self._get_model_config(ds_model)
        model_name = ds_model.display_name
    else:
        logger.info(f"Assuming Hugging Face model ID: Fetching config for '{model_id}'.")
        config = self._fetch_hf_config(model_id)
        model_name = model_id

    return config, model_name

return json.load(f)
except HfHubHTTPError as e:
if "401" in str(e):
raise AquaValueError(
Copy link
Member

Choose a reason for hiding this comment

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

I think HF CLI should also have some command to register token? If so we cna offer both options in the message.

config_path = hf_hub_download(repo_id=model_id, filename="config.json")
with open(config_path, "r", encoding="utf-8") as f:
return json.load(f)
except HfHubHTTPError as e:
Copy link
Member

Choose a reason for hiding this comment

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

Please check the existing - format_hf_custom_error_message, i'm wondering if it can be reused here?

environment variables.
"""
if not compartment_id:
compartment_id = os.environ.get(
Copy link
Member

Choose a reason for hiding this comment

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

in ads.config.py we have a unified - COMPARTMENT_OCID variable, maybe it an be reused?

"A compartment OCID is required to list available shapes. "
"Please provide it as a parameter or set the 'NB_SESSION_COMPARTMENT_OCID' "
"or 'PROJECT_COMPARTMENT_OCID' environment variable."
"cli command: export NB_SESSION_COMPARTMENT_OCID=<NB_SESSION_COMPARTMENT_OCID>"
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to give AQUA CLI command that will include compartment as an input parameter:

ads aqua deployment recommend_shape --model_id "<OCID>" --compartment_id "<OCID>"

Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.26%

mrDzurb
mrDzurb previously approved these changes Sep 16, 2025
Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.22%

Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.22%

Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.22%

Copy link

📌 Cov diff with main:

Coverage-70%

📌 Overall coverage:

Coverage-58.42%

@Aryanag2 Aryanag2 merged commit 9e141cb into main Sep 18, 2025
24 checks passed
Copy link

📌 Cov diff with main:

Coverage-70%

📌 Overall coverage:

Coverage-58.42%

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.

3 participants