Skip to content

[rocjitsu][ci] Update benchmark workflow for the sim perf dashboard - #13663

Merged
kuhar merged 15 commits into
developfrom
users/RattataKing/dashboard-workflow-refresh
Oct 21, 2026
Merged

kuhar merged 15 commits into
developfrom
users/RattataKing/dashboard-workflow-refresh

Conversation

@RattataKing

Copy link
Copy Markdown
Contributor

@RattataKing
RattataKing requested review from a team and atgutier as code owners October 16, 2026 19:00
@RattataKing
RattataKing requested a review from kuhar October 16, 2026 19:00
@RattataKing RattataKing assigned IanWood1 and unassigned IanWood1 Oct 16, 2026
@RattataKing
RattataKing requested a review from IanWood1 October 16, 2026 19:01
@github-actions github-actions Bot added github actions Pull requests that update GitHub Actions code project: rocjitsu labels Oct 16, 2026
@RattataKing
RattataKing force-pushed the users/RattataKing/dashboard-workflow-refresh branch from 2b49048 to 5904288 Compare October 16, 2026 19:01
@therock-pr-bot

therock-pr-bot Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

  • python -m pip install pre-commit
  • pre-commit install
  • pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

@therock-pr-bot

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we remove the unused scaffolding so these interfaces show what is actually exercised?

  • In emulation/rocjitsu/website/dashboard/scripts/dashboard-data/tests/test_benchmark_result.py:5,63,85, importlib.util and manifest() are unused, and context() is consumed only for source_subject. Simplifying that fixture also removes unused revision/context fields that look as though they are being checked.
  • At .github/workflows/rocjitsu-benchmarks.yml:44, the machine and source_commit_message job outputs have no downstream consumers; their step outputs already feed the converter locally.
  • In the test file at line 235, the removed-script-name blacklist mostly records intermediate implementation choices. Assertions about the intended publication artifacts and validation would protect the contract more directly.

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left 3 inline comments

@RattataKing
RattataKing marked this pull request as draft October 16, 2026 20:59

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would help to make these helper names and test fixtures more explicit:

  • In emulation/rocjitsu/website/dashboard/scripts/dashboard-data/prepare-benchmark-result.py:29,33, use names such as parse_timestamp and build_published_run: the former parses a datetime, and the latter builds a whole run, whereas the dashboard contract uses “result” for a per-test record.
  • In emulation/rocjitsu/website/dashboard/scripts/dashboard-data/tests/test_benchmark_result.py:15, name ROOT as REPO_ROOT to identify which root the seven-parent traversal targets.
  • In the same test file at line 104, explain that the unusual indentation and trailing whitespace deliberately detect reserialization. The publication tests require the archived raw JSON to preserve those bytes.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Oct 19, 2026
@RattataKing
RattataKing marked this pull request as ready for review October 19, 2026 16:31
@RattataKing
RattataKing requested a review from kuhar October 19, 2026 16:31

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would help to make these remaining names more explicit:

  • In emulation/rocjitsu/website/dashboard/scripts/dashboard-data/tests/test_benchmark_result.py:411, rename test_workflow_shell_and_removed_paths to something like test_workflow_shell_and_publication_policy; its body now checks shell syntax, publication policy, dispatch inputs, and checkouts, with no removed-path assertions.
  • In that test file at lines 18–21, use SOURCE_SHA (or ROCJITSU_SHA) and CORPUS_SHA for SHA and CORPUS. With separate workflow and Tensile revisions in the fixture, identifying each revision at its declaration makes the provenance assertions easier to follow.
  • In emulation/rocjitsu/website/dashboard/scripts/dashboard-data/prepare-benchmark-result.py:23, a name such as encode_json_bytes would make the format and return type of encoded clear at its hashing and file-writing call sites.

@RattataKing
RattataKing requested a review from kuhar October 19, 2026 18:08

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left an inline reply and 3 new comments

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few comments, plus an inline reply

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left some inline comments

@RattataKing
RattataKing requested a review from kuhar October 20, 2026 20:46

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left an inline reply and a new comment

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@kuhar
kuhar merged commit 0152496 into develop Oct 21, 2026
177 of 182 checks passed
@kuhar
kuhar deleted the users/RattataKing/dashboard-workflow-refresh branch October 21, 2026 17:00
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation github actions Pull requests that update GitHub Actions code organization: ROCm project: rocjitsu

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants