-
Notifications
You must be signed in to change notification settings - Fork 59
[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.
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
…ated-data-science into WIP/handle-params
…ence into WIP/handle-params
| config_parameters = item.parameters.get( | ||
|
|
||
| # If user DID NOT provide specific params (None or Empty), we look for defaults | ||
| if not user_params: |
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.
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.
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 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
|
Note: --served-model-name odsc-llm --disable-custom-all-reduce --seed 42 --trust-request-chat-template will be present in all scenarios since they are container level params All test cases are for multi model deployment of Llama-3.2-3B on A10.2 shape Scenarios 1 & 2: Default (Case 1) vs. Explicit Clear (Case 2) Llama_Default2: No params provided (None). Expected to load SMM defaults. Llama_Clear2: Empty params provided ({}). Expected to not load SMM defaults. Verification Results: Llama_Default2: Success. Contains --max-model-len 65536. Llama_Clear2: Success. Does not contain --max-model-len. Scenario 3: User Override (Case 3) Llama_Override: Success. Used 1024. Default 65536 was NOT merged in. |
|
|
||
| if user_params: | ||
| # Validate the resolved parameters | ||
| if deployment_params: |
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.
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
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.
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.
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 see, so in the case of defaut config, this should raise service error with clear wording instead of aqua value error?
|
|
||
| if user_params: | ||
| # Validate the resolved parameters | ||
| if deployment_params: |
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.
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.



Improve Handling of Default SMM Parameters for Model Deployment (UI & Backend)