-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: add build_model function #584
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.
minor comments
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.
- Also update changelog
- Include a unit test for this method
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 some comments
@pytest.mark.parametrize( | ||
'descriptor, select_model, is_onnx, expect_error', | ||
[ | ||
('bert-base-cased', None, False, None), | ||
('bert-base-cased', None, True, None), | ||
('openai/clip-vit-base-patch16', 'clip-text', False, None), | ||
('openai/clip-vit-base-patch16', 'clip-vision', False, None), | ||
('openai/clip-vit-base-patch16', None, False, SelectModelRequired), | ||
('MADE UP MODEL', None, False, ValueError), | ||
], | ||
) |
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.
no need to change, but a better way is
@pytest.mark.parametrize( | |
'descriptor, select_model, is_onnx, expect_error', | |
[ | |
('bert-base-cased', None, False, None), | |
('bert-base-cased', None, True, None), | |
('openai/clip-vit-base-patch16', 'clip-text', False, None), | |
('openai/clip-vit-base-patch16', 'clip-vision', False, None), | |
('openai/clip-vit-base-patch16', None, False, SelectModelRequired), | |
('MADE UP MODEL', None, False, ValueError), | |
], | |
) | |
@pytest.mark.parameterize('is_onnx', [True, False]) | |
@pytest.mark.parametrize( | |
'descriptor, select_model, expect_error', | |
[ | |
('bert-base-cased', None, None), | |
('openai/clip-vit-base-patch16', 'clip-text', None), | |
('openai/clip-vit-base-patch16', 'clip-vision', None), | |
('openai/clip-vit-base-patch16', None, SelectModelRequired), | |
('MADE UP MODEL', None, ValueError), | |
], | |
) |
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 more you know!
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!
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.
One minor thing
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.
.
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!
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 too comments, rather minor
finetuner/__init__.py
Outdated
select_model: Optional[str] = None, | ||
device: Optional[str] = None, | ||
is_onnx: bool = False, | ||
): |
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.
Here is an annotation of the return type missing
else: | ||
model = finetuner.build_model( | ||
name=descriptor, select_model=select_model, is_onnx=is_onnx | ||
) |
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.
if build_model returns nothing it would actually pass. Testing something with the model would be better. Maybe you can just check if you can pass an input and the result is a vector of the expected shape. If this is too much, you could at least check if the type of model
is correct and not None
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, but I think documentation for it is still missing, but you can do this in a separate PR.
This PR adds a
build_model
using features added in pr 336.