Refine agent contribution guidance#1488
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR centralizes ModelOpt's development guidance under a new ChangesDevelopment Guidance Centralization
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
realAsma
requested review from
a team and
danielkorzekwa
and removed request for
a team
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1488 +/- ##
==========================================
- Coverage 76.78% 76.60% -0.18%
==========================================
Files 473 473
Lines 51413 51545 +132
==========================================
+ Hits 39476 39488 +12
- Misses 11937 12057 +120
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Docs-only refactor that pulls agent guidance out of CLAUDE.md into a shared .agents/README.md plus developer-guidelines.md, with thin entrypoints for AGENTS.md and CLAUDE.md. Direction is good (single source of truth, link out instead of duplicate). A few improvement opportunities worth a look before merge — none are blockers:
-
AGENTS.mduses Claude-specific@include syntax.@.agents/README.mdis interpreted as an inclusion directive by Claude Code, butAGENTS.mdis the cross-tool convention (Codex, Cursor, Aider, etc.) and those tools will treat@.agents/README.mdas literal text. Consider either inlining the content or using a plain markdown link plus a one-line "agents should read this file" instruction so non-Claude tools get the guidance. -
A few useful items from the old
CLAUDE.md"Rules (Read First)" section don't survive the link-out. Specifically: the warning "do NOT substitute the harness's default## Summary/## Test planformat" when filling in the PR template, the "run/claude reviewfor non-trivial PRs" guidance, "re-run tests locally after rebase", and "never attribute AI tools in sign-off". These are agent-specific behaviors that aren't restated inCONTRIBUTING.md, so linking there doesn't recover them. Worth adding a short "Agent-specific PR conventions" bullet list in.agents/README.md. -
CLAUDE.local.mdoverride hint moved out of the shared file. OnlyCLAUDE.mdmentions it now; agents reading viaAGENTS.md→.agents/README.mdwon't know about the local-overrides pattern. If the pattern is meant to be tool-agnostic (e.g.AGENTS.local.md), surface it in.agents/README.md; if it's Claude-only, that's fine as-is. -
Code-areas prose in
.agents/README.mdis less complete than the old table. It omitsnas,peft,kernels,_deploy, andutils. Minor — but if the goal is "linked entrypoint", a one-line link to a fuller listing (or restoring the table) would be more useful than a partial enumeration. -
CONTRIBUTING.mdnox example pulls inmegatron-core.partial_unit-3.12(torch)installs megatron-core, which is heavyweight for what the section frames as "focused local validation". The pytest example just above already covers the lightweight case; consider whether the nox example here should point at a lighter session, or add a note that this session has heavier deps.
No correctness or licensing issues. Tests N/A (docs-only).
| @@ -0,0 +1,3 @@ | |||
| # AGENTS.md | |||
|
|
|||
| @.agents/README.md | |||
There was a problem hiding this comment.
Bot comment.
@.agents/README.md is Claude Code's include syntax. AGENTS.md is the cross-tool spec (Codex, Cursor, Aider, etc.) and those tools will render this as literal text rather than expanding the include — agents using them won't see the guidance. Consider inlining the content here, or replacing with a plain markdown link plus a sentence telling the agent to read it (e.g. Agents working in this repo should read [.agents/README.md](.agents/README.md) before making changes.).
| - **Contributing and PRs:** Read `CONTRIBUTING.md` for commit conventions, | ||
| DCO sign-off, signing, PR expectations, and review requirements. | ||
|
|
||
| - **PR readiness:** Before opening or marking a PR ready for review, read |
There was a problem hiding this comment.
Bot comment.
A few items from the old CLAUDE.md Rules section don't carry over by linking to CONTRIBUTING.md: the warning to fill in .github/PULL_REQUEST_TEMPLATE.md verbatim and not substitute the harness's default ## Summary / ## Test plan format, running /claude review for non-trivial PRs, re-running tests after rebase, and "never attribute AI tools in the sign-off line". These are agent-specific behaviors not restated in CONTRIBUTING.md. Worth keeping a short "Agent PR conventions" bullet list here.
| | Example/integration tests | `tests/examples/` | Integration tests for examples; see `tests/examples/README.md` | | ||
| | Pre-commit / lint | `.pre-commit-config.yaml` | ruff, mypy, clang-format, license headers, bandit | | ||
| | Coverage | `pyproject.toml` | 70% minimum on `modelopt/*` | | ||
| If `CLAUDE.local.md` exists, read it after `.agents/README.md` for local overrides. |
There was a problem hiding this comment.
Bot comment.
The CLAUDE.local.md override hint is only visible to Claude users now. If the local-overrides pattern is meant to be tool-agnostic, consider moving (or duplicating) it into .agents/README.md so agents reaching it via AGENTS.md also know about it.
| `modelopt.torch.speculative` covers speculative decoding; and | ||
| `modelopt.torch.export` covers export flows. `modelopt.torch.prune`, | ||
| `modelopt.torch.distill`, and `modelopt.torch.sparsity` cover other torch | ||
| algorithms. `modelopt.onnx`, `modelopt.deploy`, and `modelopt.recipe` cover |
There was a problem hiding this comment.
Bot comment.
This enumeration drops nas, peft, kernels, _deploy, and utils that the old table covered. Either restore them, or replace the prose with a single link to a fuller listing — a partial enumeration is a bit of a trap (agents may assume there's nothing else).
| For standard repo sessions and dependency setup, use [noxfile.py](./noxfile.py). Run `nox -l` to list available sessions, then run the matching session with `nox -s <session>`. For example, `partial_unit` covers the broader torch unit test suite, including areas such as quantization: | ||
|
|
||
| ```bash | ||
| nox -s "partial_unit-3.12(torch)" |
There was a problem hiding this comment.
Bot comment.
partial_unit-3.12(torch) installs megatron-core as part of the session, which is fairly heavy for what this section frames as "focused local validation". The plain pytest example above already covers the lightweight path; consider either pointing at a lighter nox session or noting the heavier deps so users aren't surprised.
There was a problem hiding this comment.
megatron-core is a very lightweight package - takes few seconds only to install. Some features will still be disabled if they require other heavy dependencies like TE
But yes we can also point to just unit-3.12(torch_211, tf_latest)
Signed-off-by: realAsma <akuriparambi@nvidia.com>
realAsma
force-pushed
the
asma/modelopt_agent
branch
from
7cc5739 to
85fd1fb
Compare
Signed-off-by: realAsma <akuriparambi@nvidia.com>
Review follow-up after the latest head:
|
There was a problem hiding this comment.
can you also see our claude bot review prompt here to see if it points to right set of files for its review? https://github.com/NVIDIA/Model-Optimizer/blob/main/.github/workflows/claude_review.yml
kevalmorabia97
requested review from
ChenhanYu,
chadvoegele,
kaix-nv,
meenchen,
mxinO and
shengliangxu
and removed request for
danielkorzekwa
3 participants
What does this PR do?
Type of change: documentation.
This PR adds repo-local production code guidelines for agents and simplifies the agent entrypoint.
The main intent is to keep
.agents/README.mdsmall: it links agents to the relevant repo docs and code areas instead of duplicating long command tables, architecture summaries, or policy text. The repo code and existing documentation remain the source of truth, and agents can follow the links to inspect the relevant parts for the task at hand.The agent docs should primarily capture guidance that agents cannot reliably infer from source code alone, such as production coding principles, review expectations, PR readiness, and publishing constraints.
Key changes:
.agents/developer-guidelines.md..agents/README.mdinto a concise linked entrypoint for agent workflows.CONTRIBUTING.md,SECURITY.md,.github/PULL_REQUEST_TEMPLATE.md,pyproject.toml,.pre-commit-config.yaml,noxfile.py, and relevant ModelOpt code areas instead of restating that content.CONTRIBUTING.mdfor local validation.Usage
N/A. Documentation-only change.
Testing
git diff --cached --checkmarkdownlint-cli2.Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: N/AAdditional Information
Docs-only agent guidance update.
Summary by CodeRabbit