oumi-pr-review

name

oumi-pr-review

description

Pre-empt the most-repeated maintainer feedback on PRs to the oumi-ai/oumi repository. Distilled from analysis of ~580 substantive review comments across 40 PRs from the top reviewers (wizeng23, oelachqar, kaisopos, nikg7, taenin, jgreer013). Use when reviewing a PR against oumi-ai/oumi, when working in a clone of oumi-ai/oumi and asked to "review my changes", "self-review before pushing", "what would maintainers say", "oumi review", or any PR URL on oumi-ai/oumi. Catches docstring style, exception taxonomy, fork-URL leaks, magic literals, dict-vs-dataclass, mocking discipline, tokenizer footguns, and the other patterns that maintainers repeatedly flag by hand.

Oumi PR Review

The goal of this skill is to save oumi maintainers' time by catching, before they have to, the feedback they write over and over again. Lint and CI already cover formatting/imports — focus on the semantic, taxonomic, and stylistic issues humans repeatedly flag.

Workflow

1. Establish scope. Identify the diff under review:
   • PR URL given → gh pr diff <N> --repo oumi-ai/oumi and gh pr view <N> --repo oumi-ai/oumi --json files,title,body,author
   • Local work → combine the relevant slices: git diff <base>...HEAD for committed work on the branch (default base: main), git diff --cached for staged-but-uncommitted, and git diff for unstaged. Use whichever subset matches what the user is asking you to review.
2. Build the changed-file set first. For PRs, use gh pr view <N> --repo oumi-ai/oumi --json files. For local work, take the union of the relevant slices: git diff --name-only <base>...HEAD, git diff --name-only --cached, and git diff --name-only. Repo-wide greps are only for discovery. Do not report a finding unless it lands in a changed file, and prefer changed lines when the evidence is line-local.
3. Walk the prioritized check list below in order. Stop early only if a check is irrelevant to this diff (e.g. no new tests → skip the test checks).
4. If a grep finds a pre-existing issue outside the diff, ignore it. This skill is for PR review, not repo janitoring.
5. Report findings in the format described in Output Format at the bottom. Cite file:line for every finding.
6. Do not be performative. Skip a check rather than invent a finding. Empty result is a valid result.
7. When in doubt about whether something is a real oumi convention, consult references/conventions.md. When you need a verbatim example of how a maintainer phrases a given critique, consult references/examples.md.

High-Leverage Checks (run these on every PR)

Each item lists: what to look for, the regex/heuristic, and the typical maintainer comment.

C1. Personal-fork references in configs, docs, or examples

Look for: any URL or git remote pointing at github.com/<not-oumi-ai>/oumi or a contributor's fork in YAML configs, notebooks, markdown, or docstrings. Grep: rg -n 'github\.com/[A-Za-z0-9_-]+/oumi' <changed-files> | rg -v 'github\.com/oumi-ai/oumi' Action: flag and propose oumi-ai/oumi or an oumi:// path. Near-zero false-positive rate; high embarrassment cost.

C2. Docstrings — presence and style

Required on: every public class, function, and module added in the diff. Style: Google-style, third-person descriptive verb ("Gets length…", "Iterates over…", "Adds the sample…"). Imperative ("Get length…") is wrong. Reference cited by maintainers: https://google.github.io/styleguide/pyguide.html#383-functions-and-methods Heuristic: any new def or class whose body's first statement is not a string literal → missing docstring; any docstring whose first word is an imperative verb (Get, Add, Iterate, Return, Set, Create, Build, Run, Compute) → wrong style.

C3. warnings.warn() and bare print() instead of logger

Look for: bare warnings.warn( used for runtime user messaging, or print( calls in non-CLI library code. Grep: rg -n '\bwarnings\.warn\(|^\s*print\(' --type py <changed-files> Important filter: do not flag genuine deprecation paths such as warnings.warn(..., DeprecationWarning, ...) on public APIs or dataset shims. Why it matters: runtime warnings.warn usage is often replaced with logger.warning(...) in oumi, while print bypasses the project logger entirely. Action: for non-deprecation runtime messaging, suggest logger.warning(...) / logger.info(...) using the module's existing logger.

C4. New exception classes — does OumiConfigError already cover this?

Look for: any new class .*Error(Exception) or any new subclass of an existing oumi exception. Grep: rg -n '^class \w+Error\b' --type py against the diff. Action: propose reusing the current exception hierarchy (OumiConfigError, and when the semantics fit, OumiConfigTypeError / OumiConfigParsingError; deploy-specific errors such as FireworksUnsupportedHardwareError / DeployInvalidRequestError) unless the new class adds genuine semantic value. Also check: an error raised at config-load time should be OumiConfigError, but an error from "saving could fail for many reasons" probably should not be — narrow first, raise second.

C5. Magic literals

Look for: numeric constants > single digit, or non-trivial string literals, that appear in code paths a user might want to tune. Heuristic: any int ≥ 10 or string used in business logic (not error messages, not test fixtures, not log strings) sitting inline in a function body. Action: suggest promoting to a module-level UPPER_SNAKE constant; if user-tunable, propose adding it as a config field. The wizeng23 phrasing: "It's possible we may want to add this as a config param later, and it'll be more visible as a constant."

C6. Type hints — return types, and dict[str, Any] for known schemas

Look for:

• Functions added in the diff with no return-type annotation. Flag any non-trivial function lacking -> ....
• Any dict[str, Any] / Dict[str, Any] parameter or return where the keys are actually known (tool calls, JSON-schema objects, config payloads). Action: propose a concrete Union[...], dataclass, or pydantic.BaseModel (use the fully qualified name if BaseModel is otherwise ambiguous in the file).

C7. Long if blocks — invert / early-return

Look for: any if cond: whose body is longer than ~25 lines while the else (or fallthrough) is much shorter. Action: suggest inverting to if not cond: return / continue / raise. Maintainers (esp. wizeng23) write this same readability tutorial repeatedly — it should never reach them.

C8. Helper extraction

Look for: (a) a comment longer than ~2 lines explaining a single line of code; (b) a 30+ line block doing one identifiable thing inside a longer function; (c) growing collections of helper classes inside a single large module. Action: propose extracting to a named method or a sibling file. Quote jgreer013: "If a single line of code needs a comment this long, it's probably worth it to move it out into its own method."

C9. Dead / commented-out code

Look for: new commented-out code blocks (consecutive #-prefixed lines that were valid code) or unused fields/parameters. Action: delete, or replace with a short rationale comment that says why the code is gone.

C10. Tiny dataset slices in example configs

Look for: [10:20], [:5], or similarly tiny slices on a real dataset in an example or recipe config — fine in tests, never in user-facing examples. Grep: rg -n '\[\d+:\d+\]|\[:\d+\]' configs/examples/ configs/recipes/ <changed-files>

C11. Tokenizer / chat-template overrides

Look for: any code that sets model_params.chat_template, registers a custom chat template, or sets pad_token for a model family that has a built-in template (Llama, Qwen, gpt-oss, Mistral, Gemma, etc.). Action: flag for human attention; propose deferring to the model's built-in template unless the PR explicitly justifies the override. Note any new model family in tests — masking should be verified for each family.

C12. HuggingFace / TRL / vLLM code paste without license header

Look for: large new blocks where the diff comment, module docstring, or commit message references HF/transformers/trl/vllm; or near-verbatim copy of a recognizable upstream class. Action: flag the need for an Apache 2.0 attribution header at the top of the file. Quote taenin: "Be careful with copying too much HF code here. If we're blatantly copying and pasting we may need to adopt their Apache 2.0 license for these files."

C13. Tests — mocking discipline, pytest.raises, fixtures, no lone files

For any new test file or test added in the diff:

• Heavy 3rd-party deps (lm_eval, vllm, anthropic, openai) should be mocked via fixtures in unit tests, not loaded for real.
• Exception assertions use pytest.raises, not try/except or assertRaises.
• Repeated setup → shared fixture.
• A single test does not justify a new file — colocate in the existing test_<thing>.py.
• Tests should cover at least one edge / error case in addition to the happy path.
• One logical assertion per test; descriptive test name encodes the case.

C14. Concept rename — full-repo sweep

Trigger: the diff renames a public symbol, config key, file, or user-visible concept. Action: run rg -n '<old_name>' across docs/, configs/examples/, configs/recipes/, configs/projects/, tests/, notebooks/, and src/oumi/, then verify no stale references were introduced. Mention the sweep in the PR description (the maintainer will look for it).

C15. Mutating an input argument

Look for: functions that mutate a passed-in dict/list/dataclass as a side effect (and don't loudly document doing so). Action: propose returning a new value (taenin's "fruitful function with no side-effects" phrasing).

C16. Repo-aware naming

• Concrete dataset classes go in <dataset_family>_*.py, not lumped into a base_* file.
• Recipe configs include the task in the filename: 20b_lora_single_gpu_train.yaml, not 20b_config.yaml.
• A name should encode purpose. Avoid "remove" when you mean "disable".
• New names should match the local vocabulary (oumi's), not TRL's or HF's.

C17. Actionable error messages and "louder" warnings

Look for: new raise X(...) or logger.warning(...) whose message tells the user what but not why or how to fix. Action: propose adding the corrective action to the message.

What to skip — already automated

Do not waste reviewer attention (or the user's) on these — CI, ruff/black, and github-code-quality[bot] already catch them:

• Python formatting and import sorting
• Missing __eq__ overrides on dataclasses where it matters
• Unclosed file handles / missing context managers
• Trivial typo lints

Output Format

Produce a single markdown report with this structure:

## Oumi PR Review — <PR title or branch name>

### Blocking
- **[C#] <file>:<line>** — <one-line description>. <one-line rationale referencing the convention>. Suggested change: <concrete diff or wording>.

### Recommended
- **[C#] <file>:<line>** — ...

### Nits
- **[C#] <file>:<line>** — ...

### Verified clean
- C1 fork-URL refs · C3 logger usage · C13 test discipline · ...  (list which checks you ran and found nothing)

Rules for the report:

• Cite file:line on every finding. No findings without a location.
• Map each finding to a check ID (C1–C17) so the author can look up the convention.
• Severity discipline:
  • Blocking — convention violation that maintainers consistently require fixed before merge (fork URLs, missing docstrings on public API, wrong exception class, untested error path).
  • Recommended — strong preference but maintainers sometimes accept (helper extraction, magic-literal constant, type tightening).
  • Nit — style only (descriptive-verb docstring rephrasing, naming polish).
• List the "Verified clean" checks so the user knows what coverage they got — this is the proof you weren't lazy.
• No findings is a valid report. Do not pad.

When in doubt

• Detailed conventions and the exact wording maintainers use: references/conventions.md
• Verbatim quotes from real PRs (when you need to justify a finding the author pushes back on): references/examples.md