-
Notifications
You must be signed in to change notification settings - Fork 11
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
Hash model inputs instead of parameters #324
Conversation
Should we start differentiating between workloads and models in our documentation? Examples:
|
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.
Discuss architecture
…to robust_hashing
Heads up: Significant changes were made to ensure that analysis also works when we have the same models with multiple inputs AND max-depth is > 1. Here is one example of things working correctly. Note that the shapes and hashes of the "deeper" models are different when you compare the two workloads.
|
@jeremyfowers I converted this PR back to draft since I intend to change the messages displayed to the user as discussed in #324 (comment) |
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.
Looking great, this is pretty much good to go! Just doing "request changes" so that I have a chance to check the documentation addition before this merges.
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 all the brainstorming and multiple rounds of feedback! This change wound up in a really good place.
I pushed a few commits with minor copy-editing and I added your new example to CI (no functionality changes).
Closes #322
Overview
This PR stops hashing the models using weights and hashes input shapes instead.
Description
This PR closes #322 by implement the long-term solution and revert the workaround mentioned here: #316 (comment) (closing #316 in the same PR).
It also adds a test to
test/analysis.py
to prevent this kind of break in the future.Manually testing
CI tests were added. However, you may manually test this PR by running
benchit two_calls.py --analyze-only
.two_calls.py
Expected results:
Note:
Input Shape
is only displayed when a model has more than one workload