mirror of
https://github.com/NVIDIA/Model-Optimizer.git
synced 2026-10-02 03:14:52 +08:00
### What does this PR do? Type of change: documentation + tooling Consolidates the previously agent-only coding guidelines into `CONTRIBUTING.md` so both human contributors and AI agents work from the same source of truth, and tunes the `/claude review` and CodeRabbit automation so they are advisory (approve-only, never blocking). **Docs reorg** - Move the content of `.agents/developer-guidelines.md` into `CONTRIBUTING.md` as a new `📐 Coding standards` section plus a `Test design principles` subsection under the existing test section. Delete the now-redundant file. - Add two new coding principles: - **Imports at top of file** in both source and test files. In-function imports are reserved for resolving circular imports or guarding optional dependencies (e.g., TRT-LLM, Megatron-Core), with a brief comment naming the reason. - **`__all__` + `from .module import *`** in package `__init__.py` to make the public API surface explicit at the definition site and keep star-imports safe. - Update `AGENTS.md` to point at the merged location and move the agent-specific "use relative paths" rule into it. Drop the dangling reference in `claude_review.yml`. **AI Review automation** - `/claude review` (`.github/workflows/claude_review.yml`): - Step 1 of the mandatory workflow now reads prior Claude comments/reviews so the bot doesn't duplicate already-raised findings. - Add brief thoroughness directives (per-file category coverage, cross-file dataflow trace for new/modified public symbols) to push more issues into the first review pass. - Replace the ambiguous "approve if no significant issues" line with an explicit decision rule: `0 CRITICAL AND 0 IMPORTANT → approve`, regardless of SUGGESTION count. SUGGESTIONs alone never block approval. - **Never submits a formal "Request changes" review.** When issues are found, posts a `--comment` review instead. Claude review is advisory and must not block merges. - CodeRabbit (`.coderabbit.yaml`): - Enable `reviews.request_changes_workflow: true` so CodeRabbit can formally approve PRs once its comments are resolved and pre-merge checks pass. Despite the name, this setting never submits a blocking "Request changes" review. - Add a `tests/**/*.py` path_instruction encoding the imports-at-top rule, lean-tests guidance, and correct test-directory placement. ### Usage N/A — documentation and automation configuration. ### Testing - `pre-commit run` ran clean on both commits (markdownlint, YAML format, etc.). - `/claude review` workflow changes will be validated on the next PR that triggers it. - CodeRabbit `tests/**/*.py` rule and approval behavior will be exercised on the next PR touching tests. ### Before your PR is "*Ready for review*" - Is this change backward compatible?: ✅ - If you copied code from any other sources or added a new PIP dependency, did you follow guidance in `CONTRIBUTING.md`: N/A - Did you write any new necessary tests?: N/A (docs + workflow config only) - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: N/A - Did you get Claude approval on this PR?: ❌ (will run `/claude review` after PR opens) ### Additional Information The previous `.agents/developer-guidelines.md` content was not agent-specific — it described universal coding standards. Hiding it under `.agents/` discouraged human contributors from reading it. The merge into `CONTRIBUTING.md` makes it discoverable on the standard GitHub PR-creation flow. The shift to approve-only automated reviews keeps the signal (approvals when clean, inline comments when issues are found) without giving the bots formal merge-blocking authority — the existing pre-merge `custom_checks` security gates remain the hard blockers. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Enhanced contribution guidelines with comprehensive coding standards and new test design principles. * Updated agents guidance to remove the legacy developer-guidelines reference. * **Chores** * Enabled formal "request changes" workflow and added test-path review rules in CI config. * Refined GitHub Actions review workflow (timeout, review prompt, single-pass requirements, and approval criteria). * Removed legacy developer-guidelines file. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/Model-Optimizer/pull/1519?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Keval Morabia <28916987+kevalmorabia97@users.noreply.github.com>
59 lines
3.3 KiB
YAML
59 lines
3.3 KiB
YAML
# Reference: https://docs.coderabbit.ai/getting-started/yaml-configuration
|
|
language: "en-US"
|
|
reviews:
|
|
profile: chill
|
|
collapse_walkthrough: true
|
|
poem: false
|
|
# Allow CodeRabbit to formally approve once its comments are resolved and pre-merge checks pass
|
|
request_changes_workflow: true
|
|
path_instructions:
|
|
- path: "modelopt/**/*.py"
|
|
instructions: &security_instructions |
|
|
Review all modelopt package and examples Python changes against the security coding practices in
|
|
SECURITY.md. Flag any of the following as CRITICAL security issues,
|
|
request changes, and fail the check if ANY are present:
|
|
1. torch.load(..., weights_only=False) with no inline comment justifying why it is safe
|
|
(e.g. confirming the file is internally-generated and not user-supplied).
|
|
2. numpy.load(..., allow_pickle=True) with no inline comment justifying why it is safe.
|
|
Should expose allow_pickle as a caller-configurable parameter defaulting to False, not hardcode True.
|
|
3. trust_remote_code=True hardcoded for transformers model or tokenizer loading.
|
|
Code should expose it as a caller-configurable parameter defaulting to False, not hardcode True.
|
|
4. eval() or exec() on any input that could originate from outside the process.
|
|
5. Any use of "# nosec" comments to bypass Bandit security checks is not allowed.
|
|
If a security-sensitive pattern is genuinely necessary, the PR must be reviewed and approved
|
|
by @NVIDIA/modelopt-setup-codeowners with an explicit justification in the PR description.
|
|
6. Any addition of new PIP dependencies in pyproject.toml or requirements.txt that are not
|
|
permissive licenses (e.g. MIT, Apache 2) must be reviewed and approved by
|
|
@NVIDIA/modelopt-setup-codeowners with an explicit justification in the PR description.
|
|
- path: "examples/**/*.py"
|
|
instructions: *security_instructions
|
|
- path: "tests/**/*.py"
|
|
instructions: |
|
|
Verify tests follow the conventions in CONTRIBUTING.md. Flag the following as
|
|
IMPORTANT issues:
|
|
1. Imports inside functions or test methods without explicit justification.
|
|
Imports belong at the top of the file so import errors surface at collection
|
|
time, not mid-test. The only acceptable in-function imports are for circular
|
|
imports or optional dependencies (e.g., TensorRT-LLM, Megatron-Core), and
|
|
those should carry a brief comment naming the reason.
|
|
2. Redundant lower-level tests that duplicate behavior already covered by a
|
|
higher-level test — checked-in tests should be lean and document expected
|
|
behavior, protect against regressions, or flag backward-incompatible changes.
|
|
3. Tests placed in the wrong directory for their cost profile (e.g., multi-minute
|
|
tests under tests/unit, which targets a few-seconds budget; GPU-requiring
|
|
tests under tests/unit instead of tests/gpu*).
|
|
auto_review:
|
|
auto_incremental_review: true
|
|
drafts: false
|
|
base_branches: ["main", "release/.*", "feature/.*"]
|
|
pre_merge_checks:
|
|
custom_checks:
|
|
- name: "Security anti-patterns"
|
|
mode: "error"
|
|
instructions: *security_instructions
|
|
knowledge_base:
|
|
code_guidelines:
|
|
filePatterns:
|
|
- "CONTRIBUTING.md"
|
|
- "SECURITY.md"
|