-
Notifications
You must be signed in to change notification settings - Fork 5
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
[DEVX-219]: Model Training #208
Conversation
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 good to me.
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.
Great Work. Added some comments
clarifai/utils/model_train.py
Outdated
dict_response = MessageToDict(response) | ||
for model_type in dict_response['modelTypes']: | ||
if model_type['id'] == model_type_id: | ||
for modeltypefield in model_type['modelTypeFields']: | ||
if modeltypefield['path'].split('.')[-1] == param: | ||
if param == 'template': | ||
del modeltypefield['placeholder'] | ||
del modeltypefield['modelTypeEnumOptions'] | ||
return modeltypefield | ||
modeltypefield['param'] = modeltypefield.pop('path').split('.')[-1] | ||
del modeltypefield['placeholder'] | ||
return modeltypefield | ||
if modeltypefield['path'].split('.')[-1] == "template": | ||
for modeltypeenum in modeltypefield['modelTypeEnumOptions']: | ||
if modeltypeenum['id'] == template: | ||
for modeltypeenumfield in modeltypeenum['modelTypeFields']: | ||
if modeltypeenumfield['path'].split('.')[-1] == param: | ||
modeltypeenumfield['param'] = modeltypeenumfield.pop('path').split('.')[-1] | ||
del modeltypeenumfield['placeholder'] | ||
return modeltypeenumfield |
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.
Some comments could be helpful to maintain in future
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.
Sure. We can add comments as parsing the API response Multimodeltypes.
But every loop detail, not sure, its just indexing. Thats why kept the iter variable names clearly.
from google.protobuf.struct_pb2 import Struct | ||
|
||
|
||
def response_to_templates(response: MultiModelTypeResponse, model_type_id: str) -> List[str]: |
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.
def response_to_templates(response: MultiModelTypeResponse, model_type_id: str) -> List[str]: | |
def _response_to_templates(response: MultiModelTypeResponse, model_type_id: str) -> List[str]: |
Shall we mark all these function as internal use ? wdyt
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 functions are stored in internal utils and we are following the same structure in Workflows. So I think this is fine.
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.
lgtm!
What
Features
Model Training
Model Training support functionalities
Dataset functionalities added
TODO