Skip to content

Conversation

ahadnagy
Copy link
Contributor

@ahadnagy ahadnagy commented Sep 5, 2025

What does this PR do?

This PR is a follow-up to #40486 that adds the workflows and HF Datasets upload to start collecting data.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@ahadnagy ahadnagy marked this pull request as draft September 5, 2025 11:56
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@ahadnagy ahadnagy force-pushed the benchmarking-gh-actions branch 2 times, most recently from 594ce6d to 93d2a90 Compare September 5, 2025 12:18
@ahadnagy ahadnagy force-pushed the benchmarking-gh-actions branch from 0b88e75 to 1deb38e Compare September 5, 2025 13:52
@ahadnagy ahadnagy force-pushed the benchmarking-gh-actions branch 6 times, most recently from 4857630 to a985884 Compare September 6, 2025 18:54
@ahadnagy ahadnagy force-pushed the benchmarking-gh-actions branch from a985884 to fdc4301 Compare September 6, 2025 18:58
@ahadnagy ahadnagy changed the title [WIP] Benchmarking v2 GH workflows Benchmarking v2 GH workflows Sep 8, 2025
@ahadnagy ahadnagy marked this pull request as ready for review September 8, 2025 09:13
@ahadnagy
Copy link
Contributor Author

ahadnagy commented Sep 8, 2025

The workflow has been tested, and has a complete run here:
https://github.com/huggingface/transformers/actions/runs/17528088273

@Rocketknight1
Copy link
Member

cc @McPatate

description: 'Model ID to benchmark (e.g., meta-llama/Llama-2-7b-hf)'
required: false
type: string
default: ''
Copy link
Member

Choose a reason for hiding this comment

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

Should we put a default for the model_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's no input provided here, it runs all the registered model, so an empty input is fine here. It's just for filtering purposes in case we need it for development.

description: 'HuggingFace Dataset to upload results to (e.g., "org/benchmark-results")'
required: false
type: string
default: ''
Copy link
Member

Choose a reason for hiding this comment

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

No default here as well?

Copy link
Contributor Author

@ahadnagy ahadnagy Sep 9, 2025

Choose a reason for hiding this comment

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

Currently all the vendors will have their own repos, so we can't really provide a sensible default here. (also wouldn't make sense to make it required in case push is disabled)

required: false
type: string
default: ''
benchmark_repo_id:
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this redundant with upload_to_hub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the other has the wrong description, it's a boolean to toggle the upload on/off. Fixing it.

Comment on lines +94 to +96
if [ -n "${{ inputs.model_id }}" ]; then
args="$args --model-id '${{ inputs.model_id }}'"
fi
Copy link
Member

Choose a reason for hiding this comment

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

What would this default to if not specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll run all the registered benchmarks.

Comment on lines +7 to +9
push:
branches:
- run-benchmarking-gh-actions*
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't using labels be easier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just mirrored here what the test pipelines offer on this front.

python run_benchmarks.py --upload-to-hf username/benchmark-results

# Upload with a custom run ID for easy identification
python run_benchmarks.py --upload-to-hf username/benchmark-results --run-id experiment_v1
Copy link
Member

Choose a reason for hiding this comment

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

Do we generate and print the run_id if not provided by the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

run_id in that case becomes the benchmark_id generated earlier.

Comment on lines +231 to +234
github_run_number = os.getenv("GITHUB_RUN_NUMBER")
github_run_id = os.getenv("GITHUB_RUN_ID")
if github_run_number and github_run_id:
run_id = f"{github_run_number}-{github_run_id}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
github_run_number = os.getenv("GITHUB_RUN_NUMBER")
github_run_id = os.getenv("GITHUB_RUN_ID")
if github_run_number and github_run_id:
run_id = f"{github_run_number}-{github_run_id}"
github_run_number = os.getenv("GITHUB_RUN_NUMBER")
github_run_id = os.getenv("GITHUB_RUN_ID")
if github_run_number and github_run_id:
run_id = f"{github_run_number}-{github_run_id}"
else:
run_id = uuid.uuid4()

wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

or str(uuid.uuid4())[:8] as you used below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahadnagy ahadnagy requested a review from McPatate September 9, 2025 18:05
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.

4 participants