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

extend exrmetrics #1968

Merged
merged 3 commits into from
Feb 2, 2025
Merged

Conversation

peterhillman
Copy link
Contributor

Adds better support for benchmarking to exrmetrics, including features from #1956:

  • optionally write to memory instead of output file.
  • optionally perform multiple passes and output per-run timing and overall statistics
  • multiple input files can be specified, to allow for exrmetrics /path/to/testimages/*.exr
  • optional CSV output mode instead of (more detailed) JSON mode
  • reads/writes all parts of a file, unless a single part is specified
  • can now iterate over compression types and pixel data types (half, float, mixed types, original types)
  • supports rereading the data written, and outputs profile information for that. This simplifies testing write and read performance of different compression types, which previously needed separate calls to write then re-read the file
  • shortcut flag for creating a copy of a file with a different codec, compression level, or pixel type
  • shortcut flag for recommended benchmarking settings

The JSON output code is a little messy because it avoids adding a library dependency. It's possible certain outputs are invalid JSON due to missing commas or badly escaped strings, but the python test does try to parse the output.

Note this changes the JSON format of the output. Also, now requires -o for output file, since multiple input files are supported. Any old scripts calling exrmetrics inputfile outputfile will now not overwrite outputfile. If outputfile already exists, it will be treated as an input and analyzed, otherwise an error will be reported. The new syntax is exrmetrics inputfile -o outputfile

Signed-off-by: Peter Hillman <[email protected]>
@kdt3rd
Copy link
Contributor

kdt3rd commented Jan 31, 2025

@peterhillman these changes largely look fine to me, I did just have to rebase my PR against current main to get the CI to go through... [edit: I take it back, the rebase didn't actually fix the issue .... grrrrrr]

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

It all looks very helpful.

@cary-ilm
Copy link
Member

cary-ilm commented Feb 2, 2025

Yes, the fuzz test failure is because of the deprecated upload-artifact.

@peterhillman, we should add a description to website/bin/exrmetrics.rst.

@cary-ilm cary-ilm merged commit f7a6cb4 into AcademySoftwareFoundation:main Feb 2, 2025
32 of 33 checks passed
@lji-ilm
Copy link

lji-ilm commented Feb 3, 2025

That's impressive that you made the json output by hand!

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.

5 participants