Skip to content

fix(report): surface qps/tps/seeds after git_sha; track package git_sha#377

Draft
nvzhihanj wants to merge 1 commit into
mainfrom
fix/report-fieldorder-gitsha
Draft

fix(report): surface qps/tps/seeds after git_sha; track package git_sha#377
nvzhihanj wants to merge 1 commit into
mainfrom
fix/report-fieldorder-gitsha

Conversation

@nvzhihanj

Copy link
Copy Markdown
Collaborator

Summary

Two fixes to the benchmark report (Report / result_summary.json).

1. qps/tps/seeds surfaced after git_sha

They previously serialized at the bottom of result_summary.json (msgspec serializes in field-definition order and defaulted fields must come last). Report.to_json now reorders the serialized mapping so the head reads version, git_sha, qps, tps, seeds, …, with the long metric dicts trailing. Reordered at serialization time (not the struct), so no impact on the existing direct-Report(...) construction tests.

2. git_sha now reflects the endpoints package, not the caller's cwd

get_git_sha() ran git rev-parse in the process cwd, so a benchmark launched from an unrelated git repo (e.g. a mounted workdir) recorded that repo's SHA. Now:

  • anchored to the package directory — dev/editable checkouts report the endpoints HEAD regardless of cwd;
  • reads _git_archival.txt, substituted by git archive export-subst (sdist/container builds carry no .git) — verified the substitution works;
  • resolution order: baked archival SHA → package-anchored git rev-parseNone (never a wrong cwd SHA).

Tests

  • to_json key order (qps/tps right after git_sha).
  • git_sha: archival substitution, placeholder-never-leaks, cwd-independence.
  • 27 unit tests pass; ruff format clean.

Based on main (PR #372 still open; whoever merges it has a trivial to_json/report.py resolution).

🤖 Generated with Claude Code

@nvzhihanj nvzhihanj requested a review from a team June 26, 2026 17:40
@github-actions

Copy link
Copy Markdown

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

@github-actions github-actions Bot requested a review from arekay-nv June 26, 2026 17:40
@nvzhihanj nvzhihanj marked this pull request as draft June 26, 2026 17:41

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces a mechanism to resolve the package's git commit SHA using git archive's export-subst feature via a new _git_archival.txt file, falling back to running git rev-parse anchored to the package directory. It also reorders the serialized JSON output of metrics reports to surface key throughput and run identity fields first. Feedback is provided regarding a potential issue where git rev-parse could traverse up to a parent repository if the package is installed in a virtual environment within an unrelated git repository, along with a suggestion to check for the existence of the .git directory first.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 49 to 51
try:
result = subprocess.run(
["git", "rev-parse", "--short=7", "HEAD"],

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.

high

If the package is installed as a standard non-editable package (e.g., from a wheel or sdist) inside a virtual environment, and that virtual environment is located within an unrelated git repository, running git rev-parse with cwd=_GIT_ARCHIVAL.parent will traverse up the directory tree and find the parent project's .git directory. This will cause get_git_sha() to incorrectly return the host project's git SHA instead of None.

To prevent this, we should verify that the package's own repository root contains a .git directory/file before attempting to run git rev-parse.

Suggested change
try:
result = subprocess.run(
["git", "rev-parse", "--short=7", "HEAD"],
if not (_GIT_ARCHIVAL.parent.parent.parent / ".git").exists():
return None
try:
result = subprocess.run(
["git", "rev-parse", "--short=7", "HEAD"],

nvzhihanj added a commit that referenced this pull request Jun 26, 2026
…sha hack

Pivot to dynamic VCS versioning per review on #377:
- build backend uv_build -> hatchling + hatch-vcs; version is now `dynamic`,
  derived from git (nearest tag + commits-since + short SHA), written to
  src/inference_endpoint/_version.py at build time (gitignored). __version__ reads it.
- the commit SHA is embedded in the version string itself (e.g. 0.6.dev3+g6eac351),
  so the report's version is self-identifying for submitters; the stale hardcoded
  0.1.0 (unchanged since the first commit) is gone.
- get_git_sha now just parses the +g<sha> local segment from __version__ — drops the
  cwd-dependent `git rev-parse` and the hand-rolled _git_archival.txt parsing.
- git-archive/sdist/container builds (no .git) recover the version via the standard
  setuptools_scm .git_archival.txt export-subst.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nvzhihanj added a commit that referenced this pull request Jun 27, 2026
Per review on #377:
- Drop the result_summary.json key reordering (qps/tps after git_sha) — hard to
  maintain; Report.to_json goes back to plain field-order serialization.
- Add a 'loadgen' field to Report: the run's load-generation settings (benchmark
  mode, load pattern, target qps/concurrency, n_samples, duration bounds, workers),
  sourced from config at finalize like 'seeds'. Serialized into result_summary.json
  and rendered as a 'Load settings:' line in report.txt.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Snapshot the run's load/runtime configuration into the report so it is
self-describing and a run is identified by its settings. A single 'run_config'
field is populated from settings.model_dump(include={runtime, load_pattern,
client, warmup}) — the Pydantic config is the single source of truth, so new or
renamed config fields propagate automatically, with no hand-listing of fields in
from_snapshot/finalize. endpoint_config (api_key/URLs) is a sibling of settings
and thus excluded — no secrets leak. Rendered as a 'Run config:' block in report.txt.

Replaces the standalone 'seeds' field (#353): the RNG seeds now live under
run_config.runtime / run_config.warmup.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nvzhihanj nvzhihanj force-pushed the fix/report-fieldorder-gitsha branch from 728749f to 4fe34b3 Compare June 27, 2026 04:37
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.

1 participant