-
Notifications
You must be signed in to change notification settings - Fork 58
[AQUA][STMD]Updated logic to deploy single ft model #1269
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
Conversation
ads/aqua/modeldeployment/entities.py
Outdated
f"Detected base model is fine-tuned model {AQUA_FINE_TUNE_MODEL_VERSION} and switched to stack deployment." | ||
) | ||
segments = aqua_fine_tuned_model.split("#") | ||
if len(segments) != 2: |
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.
Can you add some comment describing this logic.
I think we need to make it a bit more flexible. Maybe to create a method in utils somewhere that could extract base model ocid from this tag. What if tomorrow we delete this #model_name
from the tag. I thiknk we should still split by #
, but then take the first one and check if it is even ocid, if so then we are good.
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.
Updated
"Validation failed: Fine-tuned model ID '%s' is not supported for model deployment.", | ||
base_model.id, | ||
) | ||
raise ConfigValidationError( |
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'm not sure if that error makes sense. We might need to give user a better sense what's going on and how to fix the problem.
) | ||
|
||
def validate_base_model(self, model_id: str) -> None: | ||
def validate_base_model(self, model_id: str) -> Union[str, AquaMultiModelRef]: |
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 return sections is missing in the docstring.
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.
Added
Updated logic to deploy ft model as single model.
Unit
Integration