-
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
[EAGLE-3230] [EAGLE-3452] Create tests users can run for triton model upload and fix error in python 3.11 #165
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.
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.
Some minor comments, looks good
f"`labels.txt` is empty!. Model type `{self.model_type}` requires input labels in `labels.txt`" | ||
) | ||
|
||
def test_inference_with_predefined_inputs(self): |
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.
This is a good test, but I think it may be difficult for the user to debug, for two reasons:
- it's in a library class, not the test file visible to the user, so when it fails they may need to look at the package code to debug
- it's cased with lots of input types other than the one that user is using, making most of the code irrelevant to their model
Another option could be to create a different test class with this test function for each model type, in separate files. Then when generating the test repo, copy the class for the model type into the test module, so the test code is easily visible and editable (may or may not want to do this second part depending on what it looks like).
Not needed to merge, we could change that later if needed.
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.
Thank you for your suggestions. I believe users can follow the messages in the test and fix their inference code because it has messages for individual assertions.
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.
This was battle tested yesterday on the model-upload training. Some minor issues we had have been covered with the last commit.
What
How