Improve Benchmarking#2798
Conversation
JohnReedV
added
the
skip-cargo-audit
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE Baseline scrutiny: established OpenTensor-associated contributor with repo write permission; branch apply-benchmark-to-only-bad-benches -> devnet-ready. Reviewed the trusted instructions, prior Skeptic sticky, contributor signals, PR metadata, comments, changed-file list, overlap signal, and the pre-fetched diff. The prior Skeptic sticky contains no finding IDs to reconcile. The PR does not modify FindingsNo findings. ConclusionNo malicious intent or security vulnerability was found in the reviewed diff. Residual risk is limited to benchmark-tooling correctness and generated-weight accuracy, which does not cross the Skeptic security threshold here. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 Gittensor UNKNOWN; author has write permission, @opentensor profile context, and substantial recent subtensor contribution history, so this was calibrated as an established-contributor domain review. PR body is concise but substantive enough to leave unchanged. Spec version is bumped from Duplicate-work check: overlapping open PRs touch common runtime/weight files, but their titles and scopes are unrelated to benchmark drift patching or the new benchmark-coverage lint. Validation: FindingsNo findings. Prior-comment reconciliation
ConclusionThe prior matcher precision issue is addressed, the generated benchmark weight plumbing is now wired to the corresponding dispatch annotations, and the benchmark update script/lint changes are consistent with the PR description. I found no remaining blocking domain issues. 📜 Previous run (superseded)
|
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
JohnReedV
changed the title
…tensor/subtensor into apply-benchmark-to-only-bad-benches
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| let direct = format!("WeightInfo::{name}"); | ||
| let associated = format!("::{name}"); | ||
| normalized.contains(&direct) | ||
| || normalized.split("WeightInfo").skip(1).any(|suffix| { | ||
| suffix.contains(&associated) || suffix.contains(&format!("::<T>{associated}")) | ||
| }) |
There was a problem hiding this comment.
[MEDIUM] Match the complete WeightInfo method name
This accepts any WeightInfo call whose method merely starts with the dispatchable name. For example, a dispatchable named swap_coldkey would be treated as correctly plugged if its weight attribute called WeightInfo::swap_coldkey_announced(), because ::swap_coldkey_announced contains ::swap_coldkey. That is the exact class of benchmark/weight wiring mistake this lint is meant to catch, especially in this repo where prefix-related names exist. Require a call boundary after the matched method name, and add a unit test for the prefix case.
| let direct = format!("WeightInfo::{name}"); | |
| let associated = format!("::{name}"); | |
| normalized.contains(&direct) | |
| || normalized.split("WeightInfo").skip(1).any(|suffix| { | |
| suffix.contains(&associated) || suffix.contains(&format!("::<T>{associated}")) | |
| }) | |
| normalized.split("WeightInfo").skip(1).any(|suffix| { | |
| [format!("::{name}"), format!("::<T>::{name}")] | |
| .iter() | |
| .any(|needle| { | |
| suffix | |
| .split(needle.as_str()) | |
| .skip(1) | |
| .any(|rest| rest.starts_with('(') || rest.starts_with("::<")) | |
| }) | |
| }) |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
1 participant
Description
This PR
Modifies the benchmark flow so that only extrinsics that have drifted passed the threshold or have incorrect read/writes are updated rather than replacing the entire file and creating conflicts everywhere.
Adds a lint to check for extrinsics with missing benchmarks
Adds missing benchmarks
fixes a bug where it would fail extrinsics where it shouldn't.