-
Notifications
You must be signed in to change notification settings - Fork 68
feat: video models and seeding for inference #395
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
bb515c8 to
914b790
Compare
914b790 to
e73912e
Compare
1a7caeb to
7a79bd6
Compare
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 small comments, but feel free to mege after resolving,
| self.model_args = default_args | ||
| self.model_args = model_args if model_args else {} | ||
| # We want the default output type to be pytorch tensors. | ||
| self.model_args["output_type"] = "pt" |
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.
Is this intended to overwrite? Perhaps checking if it is present and overwriting only if it is not is better?
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.
Yes! In the previous implementation we already automatically transformed everything to Tensors using torchvision.transforms.toTensor (line 87), so this is me just refactoring it using the more "official" way
| ) | ||
| inference_function = getattr(self, inference_function_name) | ||
|
|
||
| self.inference_handler.model_args = filter_load_kwargs(self.model.__call__, self.inference_handler.model_args) |
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.
sassy 😂
| if output_attr != "none": | ||
| pipe_output = getattr(pipe_output, output_attr) | ||
| pipe_output = pipe_output[0] |
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.
why were we doin this again? this is only a problem when using them in tests like this, but this is not the behavior we expect users to deal with, correct?
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.
Exactly, we are doing all this here because the pipe is not a PrunaModel. Indeed when we use PrunaModel and Evaluation Agent we don't have to deal with this at all. This is the exact feature I am testing here :D
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.
only have minor comments. super clean 💅
| """ | ||
| inputs = metric_data_processor(x, gt, outputs, self.call_type) | ||
| if inputs[1].dtype == torch.bfloat16: | ||
| inputs[1] = inputs[1].to(torch.float16) |
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 would just cast it to f32, tbh.
This could actually change the value of the input, because the precision is different between bf16 and f16
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.
Actually moving this entire type casting part to inference handler! And yes will use f32 tysm
deb8d45 to
3e56fb9
Compare
Description
This PR introduces several updates to the inference handlers:
DiffusersHandler Enhancements:
Seeding Support Across Handlers:
For DiffusersHandler, seeding is crucial since it directly affects the generation process.
For other models, inference is usually deterministic, but we added seeding for reproducibility in cases where randomness is involved.
Users can explicitly set or unset the seed. By default, no seed is applied, leaving seeding control to the user.
Type of Change
How Has This Been Tested?
Unit tests were added in tests/test_handler.py to validate the new functionality:
Verified that seeding and unseeding work as expected for both standard inference handlers and the DiffusersHandler.
Added tests for image and video model outputs using lightweight/tiny models to ensure correctness and efficiency.
Checklist
Now you can configure the seed like: