Skip to content
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

[tuner] Use JSON for benchmark output #256

Merged

Conversation

mihaescuvlad
Copy link
Contributor

@mihaescuvlad mihaescuvlad commented Oct 5, 2024

Notes

  • Adds extract_benchmark_from_run_result method to help in fetching the "benchmarks" data
  • Updates IREEBenchmarkResult model to reflect that the result is no longer stored as string but as a list of benchmarks
  • Updates the parsing of dispatches and models to read from Json

Testing

  • Updates tests to verify that get_mean_time functions as expected
  • Updates tests to verify that Json data is properly parsed and processed

tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
@kuhar kuhar self-requested a review October 7, 2024 13:27
@kuhar kuhar changed the title Use JSON for benchmark output [tuner] Use JSON for benchmark output Oct 7, 2024
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
Comment on lines 193 to 222
path_config = libtuner.PathConfig()
object.__setattr__(path_config, "output_json", "output.json")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests shouldn't write files in the current working directory. We can create a temporary directory for tests artifacts if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hasn't been resolved

tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/requirements-dev.txt Outdated Show resolved Hide resolved
@mihaescuvlad mihaescuvlad force-pushed the users/mihaescuvlad/benchmark-format-json branch from c965b6d to a49ef9c Compare October 9, 2024 08:50
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this looks good but we should also account for the time unit

tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner_test.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner_test.py Outdated Show resolved Hide resolved
@mihaescuvlad mihaescuvlad force-pushed the users/mihaescuvlad/benchmark-format-json branch from a49ef9c to 761bc2e Compare October 9, 2024 14:46
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
Comment on lines 193 to 222
path_config = libtuner.PathConfig()
object.__setattr__(path_config, "output_json", "output.json")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hasn't been resolved

@mihaescuvlad mihaescuvlad force-pushed the users/mihaescuvlad/benchmark-format-json branch 3 times, most recently from 550f721 to 30ee7d7 Compare October 9, 2024 17:15
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! Just a few minor comments. I will try it out locally before approving.

tuner/tuner/libtuner.py Show resolved Hide resolved
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
tuner/tuner/libtuner_test.py Outdated Show resolved Hide resolved
@mihaescuvlad mihaescuvlad force-pushed the users/mihaescuvlad/benchmark-format-json branch from 30ee7d7 to 6c250d9 Compare October 10, 2024 20:01
tuner/tuner/libtuner.py Outdated Show resolved Hide resolved
@mihaescuvlad mihaescuvlad force-pushed the users/mihaescuvlad/benchmark-format-json branch from 6c250d9 to 20ae718 Compare October 11, 2024 10:11
@kuhar kuhar force-pushed the users/mihaescuvlad/benchmark-format-json branch from 20ae718 to 49ec0ad Compare October 11, 2024 17:20
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this on my machine and it works great. Thank you for this contribution!

@kuhar kuhar merged commit 468fb29 into nod-ai:main Oct 11, 2024
8 checks passed
monorimet pushed a commit that referenced this pull request Oct 16, 2024
### Notes
- Adds `extract_benchmark_from_run_result` method to help in fetching
the "benchmarks" data
- Updates `IREEBenchmarkResult` model to reflect that the result is no
longer stored as string but as a list of benchmarks
- Updates the parsing of dispatches and models to read from Json

### Testing
- Updates tests to verify that `get_mean_time` functions as expected
- Updates tests to verify that Json data is properly parsed and
processed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants