-
Notifications
You must be signed in to change notification settings - Fork 400
Refine agent contribution guidance #1488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
realAsma
wants to merge
2
commits into
main
Choose a base branch
from
asma/modelopt_agent
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Agent Instructions for ModelOpt | ||
|
|
||
| These instructions apply to AI-assisted work in this repository. | ||
|
|
||
| ## Repository orientation | ||
|
|
||
| - Start with `README.md` for project overview and install. | ||
| - Use `modelopt/` for source, `tests/` for focused test coverage, and | ||
| `examples/` or `docs/` for usage patterns. | ||
|
|
||
| ## Coding guidelines | ||
|
|
||
| - **Coding guide:** Code development and review require reading and following | ||
| `.agents/developer-guidelines.md`; do not skip this step. | ||
|
|
||
| ## Iterative development | ||
|
|
||
| - **Running tests:** Follow the | ||
| [writing and running tests](../CONTRIBUTING.md#-writing-and-running-tests) | ||
| instructions. For fast initial iteration, choose focused tests for the | ||
| changed area from `tests/`. | ||
| - **Running pre-commit:** Follow the | ||
| [pre-commit hook instructions](../CONTRIBUTING.md#pre-commit-hooks). Hooks may | ||
| modify files; review and re-stage those changes before committing. | ||
| - **Signed commit:** Use `git commit -s -S -m "<message>"` for commits so they | ||
| follow the [signing your work](../CONTRIBUTING.md#-signing-your-work) | ||
| requirements. | ||
| - **Never `git push` without explicit approval in the current turn.** Commit | ||
| locally is fine; publishing to a remote is not. | ||
| - After `git commit`, stop and wait for the user to say "push", "publish", | ||
| "ship", or equivalent before running `git push`, `gh pr create`, or any | ||
| push-option flags like `-o merge_request.create`. | ||
|
|
||
| ## Contributing and PR readiness | ||
|
|
||
| - Before opening or marking a PR ready for review, read the | ||
| [submitting your code](../CONTRIBUTING.md#submitting-your-code) guidance. | ||
| - Read `.github/PULL_REQUEST_TEMPLATE.md` and satisfy the checklist. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Agent Tooling Notes | ||
|
|
||
| These notes are for humans maintaining repository agent setup. They are not part | ||
| of the always-loaded agent instructions. | ||
|
|
||
| ## Shared Instructions | ||
|
|
||
| Update `.agents/README.md` for repository-wide agent instructions. The root | ||
| `AGENTS.md` and `CLAUDE.md` files are symlinked to `.agents/README.md`, so | ||
| changes there apply to both Codex and Claude Code. | ||
|
|
||
| ## Local Overrides | ||
|
|
||
| For private local instructions, use the tool-specific override file: | ||
|
|
||
| - Claude Code: `CLAUDE.local.md` is additive; it is read after `CLAUDE.md`. | ||
| - Codex: `AGENTS.override.md` replaces `AGENTS.md` in the same directory, so it | ||
| is not additive. Restate any shared instructions that should still apply. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| # Coding Principles | ||
|
|
||
| Guidelines for production code in ModelOpt. Key values: simplicity, minimalism, | ||
| and elegance. | ||
|
|
||
| ## Principles | ||
|
|
||
| - **Be surgical.** Touch the code required to solve the actual problem, whether | ||
| that is one line or a broader design change. Avoid speculative refactors, | ||
| drive-by cleanup, unrelated rewrites, and half-finished implementations. | ||
| - **Fix root causes.** Prefer the right fix over the most local patch. Do not | ||
| paper over symptoms with temporary fixes unless the temporary nature and | ||
| follow-up are explicit. | ||
| - **Design for simplicity.** Choose the design that keeps code easiest to read | ||
| and change. Put behavior at the right level, tie extensibility to known needs, | ||
| and treat heavy branching or conditional logic as bad design smells. | ||
| - **Respect ownership.** Keep behavior in the layer that owns it. Parent | ||
| abstractions should contain shared contracts and shared behavior, not | ||
| child-specific special cases. | ||
| - **Keep one source of truth.** Put shared behavior, configuration, constants, | ||
| validation, and documentation in the single place that owns them. Reuse | ||
| existing helpers and shared APIs instead of copying logic or duplicating | ||
| state. | ||
| - **Abstract to simplify.** Use helpers, base classes, registries, adapters, | ||
| plugins, or extension points when they remove real duplication, clarify | ||
| ownership, support current variation, or make call sites simpler. Do not add | ||
| abstractions for speculative future cases. | ||
| - **Make code readable at the point of use.** Names, types, and structure should | ||
| make intent clear. Keep high-level orchestration clear, move low-level | ||
| mechanics into well-named helpers when helpful, and put critical code before | ||
| helper details when local conventions allow it. | ||
| - **Comment cautiously.** Code should be clear and be the source of truth | ||
| for what happens, how it happens, and why; use comments only when the why is | ||
| not obvious from the code. First ask whether better names, clearer structure, | ||
| or simpler code can explain the intent without a comment. (Apply this guidance | ||
| to new comments only; do not rewrite or delete existing comments.) | ||
| - **Scale documentation to the API.** Higher-level and user-visible APIs deserve | ||
| useful docstrings, including examples when helpful. Lower-level internals need | ||
| docstrings only when names, types, and structure are not enough. | ||
| - **Validate at boundaries.** Check user input, files, network responses, and | ||
| external API results at the edge. Keep internal code simple by trusting types | ||
| and invariants instead of repeatedly checking for impossible states. | ||
| - **Remove touched dead code.** Delete unused imports, unreachable branches, | ||
| obsolete placeholders, stale TODOs, and debug code when they are part of the | ||
| behavior you are already touching. | ||
| - **Use workspace-relative paths.** Use relative paths in commands and file | ||
| references unless an absolute path is needed to disambiguate. | ||
|
|
||
| ## Testing | ||
|
|
||
| - **Develop with focused tests.** During development, write as many focused | ||
| tests as needed, including lower-level unit tests or internal probes, to | ||
| understand and harden behavior. | ||
| - **Curate production tests and keep them lean.** Before staging or committing, | ||
| decide which tests should be checked in. Checked-in tests should document | ||
| expected behavior, protect against regressions, or flag backward-incompatible | ||
| behavior changes. Remove redundant lower-level tests when a higher-level test | ||
| already covers the same behavior, keeping CI/CD fast and lean. | ||
|
|
||
| ## Performant AI Code | ||
|
|
||
| - **Avoid stray CPU-GPU syncs.** Tensor metadata such as `tensor.shape` is safe | ||
| to read, but scalar extraction or CPU transfers such as `tensor.item()`, | ||
| `float(tensor)`, `bool(tensor)`, `tensor.cpu()`, `tensor.numpy()`, etc. can | ||
| force CPU-GPU synchronization. Keep computation on GPU unless the CPU actually | ||
| needs the value. | ||
| - **Use rank-aware logging.** Default to `print_rank_0` instead of `print` and | ||
| `warn_rank_0` instead of generic warnings. Use per-rank output only when each | ||
| process needs to report distinct state. Generic prints and warnings clog | ||
| distributed logs. | ||
| - **Respect distributed invariants.** Avoid hidden synchronization, global state, | ||
| per-rank file races, or assumptions that only hold on a single process. | ||
|
|
||
| ## Compatibility | ||
|
|
||
| - **Preserve config and checkpoint compatibility.** Treat ModelOpt config schemas | ||
| and checkpoint formats as persisted contracts. When changing configs such as | ||
| `QuantizeConfig`, maintain backward compatibility with previous ModelOpt | ||
| checkpoints unless a breaking change is explicit and intentionally handled. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| .agents/README.md |
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| .agents/README.md |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial_unit-3.12(torch)installsmegatron-coreas part of the session, which is fairly heavy for what this section frames as "focused local validation". The plainpytestexample above already covers the lightweight path; consider either pointing at a lighter nox session or noting the heavier deps so users aren't surprised.