-
Notifications
You must be signed in to change notification settings - Fork 0
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
[SD-126] Save model summary during benchmarking #53
Conversation
… file output and cleanup
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.
Top level comment: I'd split this file into a script under power_logging and the utility functions left here.
Just so that it can be properly called from the top level package (and the instructions for it included in the README)
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.
On top of that. We should have a bash script that would run the said script for all the models and save the results to raw_data/prebuilt_models/{model_name}/model_summary.json
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.
Nice solution for getting the needed parameters. See comments below for the refactoring suggestions.
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.
On top of that. We should have a bash script that would run the said script for all the models and save the results to raw_data/prebuilt_models/{model_name}/model_summary.json
… file, added parse arguments to save_model_summary
…pyproject and created bash script for model summaries
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.
Good overall! Two more thing:
- A comment regarding input shape
- Could you run the script and add the results to DVC? Something like
dvc add raw_data/prebuilt_models && git add raw_data/prebuilt_models.dvc && dvc push -r origin
should do (ensure that you have the latest DVC data by pulling first, though)
jetson/power_logging/uv.lock
Outdated
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.
uv needs locking dependencies again, after removing torchsummary from the dependencies list
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 few suggestions but not necessary to hold this PR.
There seems to be a merge conflict with DVC tracked file.
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.
Test for this as a separate ticket would be great.
No description provided.