feat(measurement): add W&B benchmark support#191
Conversation
Greptile SummaryThis PR introduces opt-in W&B (Weights & Biases) benchmark observability to the measurement tooling, gated behind
Confidence Score: 5/5Safe to merge. W&B integration is opt-in and disabled by default; no existing production paths are altered. The change is purely additive benchmark tooling with strong defensive coding patterns throughout. Three P2 style/consistency findings were identified, none of which affect correctness or security of the default-off feature. No P0 or P1 issues were found. No files require special attention — all three findings are non-blocking style issues. Important Files Changed
|
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
7831624 to
7e360e5
Compare
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
andreatgretel
left a comment
There was a problem hiding this comment.
Thanks Aaron. I re-reviewed the current head, and all six of my previous comments are addressed.
I also looked at how the sealed SLURM import would fit RAT Bench and similar distributed runs. I do not think the full integration needs to be part of this PR. The exporter and launcher wiring, stable case identities, seal lifecycle, batch import, sacct collection, and RAT Bench-specific metrics can evolve separately in our internal experiments repo.
I left two focused comments: one correctness issue with stage aggregation and one request to support multiple SLURM jobs per logical imported case. Richer scheduler timing and resource metadata can be added later.
The existing thread about invalid W&B identifiers escaping the post-run error guard also still looks actionable.
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
|
Validated the sealed external-import flow end to end with a multi-stage SLURM case. The importer accepted seven strict measurement records, preserved multiple role-tagged SLURM job IDs, and an identical re-import was idempotent. Destination-scoped re-import also produced a distinct completed run as intended. The only correctness issue found was the native NDD |
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
|
This seems to work well overall. I validated the sealed import flow with three 100-record benchmark runs, including multi-job SLURM provenance and W&B publication. The main follow-up I noticed is that some of the generic metrics are difficult to interpret. For example, per-record mean F1 can use a different denominator for each workload and differ substantially from micro F1. I think we could address this in a focused follow-up PR without blocking this one, unless you would prefer to include it now. Some things I'm thinking of:
The underlying typed records already preserve the required counts, so this should be additive and should not require rerunning benchmarks. |
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
andreatgretel
left a comment
There was a problem hiding this comment.
Thanks Aaron. I re-reviewed the current head and resolved my previous threads. The fixes for strict NDD ingress, importer destination/publication state, and credential preflight look good. The integration remains opt-in and CI is green.
One issue before approval: test_wandb_staging_allows_repeated_basename_in_foreign_intermediate_directory resolves descriptors through /proc/self/fd, which is unavailable on macOS and causes make test to fail. Could we make this portable by comparing st_dev and st_ino with temp_root.stat()?
Non-blocking: the PR is still quite large. Reports, workspaces, and sweeps look like the cleanest candidates for a follow-up if reducing the current diff is still practical. The metric-semantics improvements discussed earlier can also remain a focused follow-up.
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Summary
Security and Privacy Boundary
extra="forbid"models. Raw text, prompts, model responses, replacement maps, entity payloads, local paths, provider secrets, and trace data cannot enter W&B payloads.WANDB_API_KEYand ambient routing variables are removed; online runs use the local W&B credential file under the preserved home directory.Slurm Import Contract
write_completion_seal.pyatomically binds case identity, Slurm provenance, producer commit, terminal status, byte and record counts, and measurement SHA-256.import_wandb_run.pyverifies the seal and snapshot before initialization, derives a destination-scoped stable run ID, and publishes imported history at deterministic step 0 withresume="allow".anonymizer-experimentslauncher patch writes seals after report completion, requires a seal forSKIP_COMPLETED, and removes stale seals before forced reruns.Reports and Sweeps
WandbRunView.Validation
make test(1025 passed)145 passed, council verifier)189 passed; two sandbox-only integration tests excluded locally)make format-checkty checkover changed measurement modulesgit diff --checkWANDB_API_KEYremovedNotes