Skip to content

[AQUA] Improve Handling of Default SMM Parameters for Model Deployment #1305

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.

Sign up for GitHub

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

Open
Aryanag2 wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[AQUA] Improve Handling of Default SMM Parameters for Model Deployment #1305

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9e7c5d6
deployment params
Aryanag2 Nov 25, 2025
e9b9a6b
Delete test_inference.py
Aryanag2 Nov 25, 2025
f894725
Delete models.json
Aryanag2 Nov 25, 2025
27c1755
mmd
Aryanag2 Nov 25, 2025
314e085
Merge branch 'WIP/handle-params' of https://github.com/oracle/acceler…
Aryanag2 Nov 25, 2025
94123be
Merge branch 'main' of https://github.com/oracle/accelerated-data-sci…
Aryanag2 Dec 2, 2025
241dfee
fixed mmd
Aryanag2 Dec 4, 2025
57de9c6
fixed unit tests
Aryanag2 Dec 5, 2025
9300052
removed temp files
Aryanag2 Dec 5, 2025
61bef3d
added unit tests for SMM changes
Aryanag2 Dec 5, 2025
1a25531
added unit tests for SMM changes
Aryanag2 Dec 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 31 additions & 11 deletions ads/aqua/modeldeployment/deployment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
build_params_string,
build_pydantic_error_message,
find_restricted_params,
get_combined_params,
get_container_env_type,
get_container_params_type,
get_ocid_substring,
Expand Down Expand Up @@ -918,10 +917,31 @@ def _create(
# The values provided by user will override the ones provided by default config
env_var = {**config_env, **env_var}

# validate user provided params
user_params = env_var.get("PARAMS", UNKNOWN)
# SMM Parameter Resolution Logic
# Check the raw user input from create_deployment_details to determine intent.
# We cannot use the merged 'env_var' here because it may already contain defaults.
user_input_env = create_deployment_details.env_var or {}
user_input_params = user_input_env.get("PARAMS")

deployment_params = ""

if user_input_params is None:
# Case 1: None (CLI default) -> Load full defaults from config
logger.info("No PARAMS provided (None). Loading default SMM parameters.")
deployment_params = config_params
elif str(user_input_params).strip() == "":
# Case 2: Empty String (UI Clear) -> Explicitly use no parameters
logger.info("Empty PARAMS provided. Clearing all parameters.")
deployment_params = ""
else:
# Case 3: Value Provided -> Use exact user value (No merging)
logger.info(
f"User provided PARAMS. Using exact user values: {user_input_params}"
)
deployment_params = user_input_params

if user_params:
# Validate the resolved parameters
if deployment_params:
Copy link
Member

@lu-ohai lu-ohai Dec 5, 2025

Choose a reason for hiding this comment

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

We need to validate find_restricted_params only if deployment_params = user_input_params. Other case may cause error like when deployment_params = config_params and its not needed as this params is not provided by user.

We should put this if deployment_params: statement under line 941.

cc: @mrDzurb

Copy link
Member

@mrDzurb mrDzurb Dec 5, 2025

Choose a reason for hiding this comment

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

We shouldn’t allow this situation. We must validate both user-provided and default params, since some system parameters are immutable. If the defaults include restricted parameters, we should throw an error and fix the default config on our side. Ideally, we’d offer a way to bypass validation, but that option doesn’t exist today.

Copy link
Member

@lu-ohai lu-ohai Dec 5, 2025

Choose a reason for hiding this comment

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

I see, so in the case of defaut config, this should raise service error with clear wording instead of aqua value error?

# todo: remove this check in the future version, logic to be moved to container_index
if (
container_type_key.lower()
Expand All @@ -935,16 +955,14 @@ def _create(
)

restricted_params = find_restricted_params(
params, user_params, container_type_key
params, deployment_params, container_type_key
)
if restricted_params:
raise AquaValueError(
f"Parameters {restricted_params} are set by Aqua "
f"and cannot be overridden or are invalid."
)

deployment_params = get_combined_params(config_params, user_params)

params = f"{params} {deployment_params}".strip()

if isinstance(aqua_model, DataScienceModelGroup):
Expand Down Expand Up @@ -1212,7 +1230,7 @@ def _create_deployment(

# we arbitrarily choose last 8 characters of OCID to identify MD in telemetry
deployment_short_ocid = get_ocid_substring(deployment_id, key_len=8)

# Prepare telemetry kwargs
telemetry_kwargs = {"ocid": deployment_short_ocid}

Expand Down Expand Up @@ -2048,9 +2066,11 @@ def recommend_shape(self, **kwargs) -> Union[Table, ShapeRecommendationReport]:
self.telemetry.record_event_async(
category="aqua/deployment",
action="recommend_shape",
detail=get_ocid_substring(model_id, key_len=8)
if is_valid_ocid(ocid=model_id)
else model_id,
detail=(
get_ocid_substring(model_id, key_len=8)
if is_valid_ocid(ocid=model_id)
else model_id
),
**kwargs,
)

Expand Down
40 changes: 24 additions & 16 deletions ads/aqua/modeldeployment/model_group_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from ads.aqua.common.utils import (
build_params_string,
find_restricted_params,
get_combined_params,
get_container_params_type,
get_params_dict,
)
Expand Down Expand Up @@ -177,26 +176,35 @@ def _merge_gpu_count_params(
model.model_id, AquaDeploymentConfig()
).configuration.get(deployment_details.instance_shape, ConfigurationItem())

final_model_params = user_params
params_found = False
for item in deployment_config.multi_model_deployment:
if model.gpu_count and item.gpu_count and item.gpu_count == model.gpu_count:
config_parameters = item.parameters.get(

# If user DID NOT provide specific params (None or Empty), we look for defaults
if not user_params:
Copy link
Member

@mrDzurb mrDzurb Dec 3, 2025

Choose a reason for hiding this comment

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

Will this cover the case where a user removes all default params in the UI? Which params will be applied then? The expectation is that SMM default params should not be used in that scenario. Basically only service params coming from the container level should be used in this case.

Copy link
Member Author

@Aryanag2 Aryanag2 Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I added a flag user_explicitly_cleared for this scenario
user_explicitly_cleared = model.params is not None and not model.params

and default params are only applied if
if not user_params and not user_explicitly_cleared:

Scenario: User clears all params in UI → UI sends "params": {}

model.params = {} (empty dict from UI)
user_params = build_params_string({}) → "" (empty string)
user_explicitly_cleared = model.params is not None and not model.params → True and True → True
if not user_params and not user_explicitly_cleared: → if True and False: → False → Skip loading SMM defaults
final_model_params = "" (stays empty, no SMM defaults loaded)
params = f"{params} {final_model_params}".strip() → just the container params

for item in deployment_config.multi_model_deployment:
if (
model.gpu_count
and item.gpu_count
and item.gpu_count == model.gpu_count
):
config_parameters = item.parameters.get(
get_container_params_type(container_type_key), UNKNOWN
)
if config_parameters:
final_model_params = config_parameters
params_found = True
break

if not params_found and deployment_config.parameters:
config_parameters = deployment_config.parameters.get(
get_container_params_type(container_type_key), UNKNOWN
)
params = f"{params} {get_combined_params(config_parameters, user_params)}".strip()
if config_parameters:
final_model_params = config_parameters
params_found = True
break

if not params_found and deployment_config.parameters:
config_parameters = deployment_config.parameters.get(
get_container_params_type(container_type_key), UNKNOWN
)
params = f"{params} {get_combined_params(config_parameters, user_params)}".strip()
params_found = True

# if no config parameters found, append user parameters directly.
if not params_found:
params = f"{params} {user_params}".strip()
# Combine Container System Defaults (params) + Model Params (final_model_params)
params = f"{params} {final_model_params}".strip()

return params

Expand Down
Loading