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

Refactor benchmark options #431

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Refactor benchmark options #431

merged 3 commits into from
Dec 12, 2024

Conversation

scotts
Copy link
Contributor

@scotts scotts commented Dec 11, 2024

Benchmark refactoring. A bunch of minor option-name changes to make them shorter. Bigger change is removing batch_parameters as an option from the core runner. The problem is that we apply it to all other benchmark scenarios - except the data loading benchmark scenario, since that is in fact a kind of batch scenario. This is confusing and not easy to extend.

We should figure out a way to generalize how to specify and run more scenarios, but I think a principle we should apply is that scenarios are self-contained. We don't want to have a scenario that implicitly applies itself to other scenarios.

For reference, the call:

python benchmarks/decoders/benchmark_decoders.py --decoders decord,decord_batch,torchcodec_public,torchcodec_core,torchaudio --min_run_seconds 20

Yields the results:

[- video=/home/scottas/github/torchcodec/benchmarks/decoders/../../test/resources/nasa_13013.mp4 h264 480x270, 13.013s 29.97002997002997fps -]
                           |  decode 10 uniform frames  |  decode 10 random frames  |  first 1 frames  |  first 10 frames  |  first 100 frames
1 threads: -----------------------------------------------------------------------------------------------------------------------------------
      DecordAccurateBatch  |            53.8            |           119.9           |       13.3       |        16.3       |        48.5      
      TorchAudio           |           183.5            |           197.4           |       11.2       |        13.1       |        33.4      
      DecordAccurate       |            52.8            |           119.0           |       13.0       |        16.3       |        66.7      
      TorchCodecCore       |            49.4            |            44.1           |       10.5       |        11.8       |        37.7      
      TorchCodecPublic     |            50.3            |            44.2           |       13.0       |        14.3       |        39.9      

Times are in milliseconds (ms).

@scotts scotts requested a review from NicolasHug December 11, 2024 22:02
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Dec 11, 2024
@scotts scotts marked this pull request as ready for review December 12, 2024 02:33
@@ -101,13 +100,13 @@ def main() -> None:
action=argparse.BooleanOptionalAction,
)
parser.add_argument(
"--bm_video_speed_min_run_seconds",
"--min_run_seconds",
Copy link
Member

@NicolasHug NicolasHug Dec 12, 2024

Choose a reason for hiding this comment

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

Nit: ideally for CLI (from the shell), all of those should be using - instead of _.
Specifying e.g. "--min-run-seconds" would still expose it as min_run_seconds in Python.

@scotts scotts merged commit 84cef50 into pytorch:main Dec 12, 2024
36 of 37 checks passed
@scotts scotts deleted the refact_bench branch December 12, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants