Skip to content

Allow run_uid#1405

Merged
dhosterman merged 1 commit into
mainfrom
accept-run_uid
Dec 4, 2025
Merged

Allow run_uid#1405
dhosterman merged 1 commit into
mainfrom
accept-run_uid

Conversation

@dhosterman
Copy link
Copy Markdown
Collaborator

CLI accepts --run-uid as an argument and will use that for the run_uid of the benchmark run instead of an automatically generated run_uid.

CLI accepts --run-uid as an argument and will use that for the run_uid
of the benchmark run instead of an automatically generated run_uid.
@dhosterman dhosterman self-assigned this Dec 3, 2025
@dhosterman dhosterman requested a review from a team as a code owner December 3, 2025 14:21
@dhosterman dhosterman temporarily deployed to Scheduled Testing December 3, 2025 14:21 — with GitHub Actions Inactive
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 3, 2025

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

Comment thread src/modelbench/record.py
run_uid: str | None,
):
_run_uid = run_uid if run_uid else f"run-{benchmark.uid}-{start_time.strftime('%Y%m%d-%H%M%S')}"
with open(json_path, "w") as f:
Copy link
Copy Markdown
Contributor

@superdosh superdosh Dec 3, 2025

Choose a reason for hiding this comment

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

I think before it was unlikely due to the timestamp, but now there's potential that json_path already exists if someone re-uses a run_uid, right? Should we check / fail out in that case?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm fine either way, but I'm also not sure I'd consider it the responsibility of modelbench to ensure that the optional run-uid argument is used responsibly.

Where would you expect that check to happen? In the cli before anything is run?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yah, that's fair, though maybe just here, it could warn if overwriting? I don't feel particularly strongly about this though!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The reason I'd be reluctant to put it here specifically is because this is after the benchmark is run, so it would kind of be awful to run a long benchmark and then puke if the path already exists.

Maybe that would be reasonable because things should be cached and a re-run would be fast? Unless the cache also uses some part of the run uid. Hmm.

Let's talk about it in standup.

@dhosterman dhosterman merged commit c7369a6 into main Dec 4, 2025
2 checks passed
@dhosterman dhosterman deleted the accept-run_uid branch December 4, 2025 15:38
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants