diff --git a/.claude/skills/design-documents/SKILL.md b/.claude/skills/design-documents/SKILL.md new file mode 100644 index 0000000000..c58eaf987c --- /dev/null +++ b/.claude/skills/design-documents/SKILL.md @@ -0,0 +1,53 @@ +--- +name: design-documents +description: Write or revise a design document under docs/design_docs/. Use when proposing a new capability, not when fixing a bug. +--- + +# Design documents + +Long is fine. A design document is scanned and returned to, not read +straight through. What must be scannable is the specification. + +- Normative statements come first in a section and stand alone. Rationale + goes in a marked block below, which a reader can skip. +- Rejected alternatives and superseded drafts go in one `Decisions` + section, cited from the places they affect. Never re-argued in place. +- A principle is stated once. Later sections cite it by name. +- Do not pre-empt objections. Drop "worth noting", "not an accident", + "deliberately", "this is not a stylistic preference". State the decision + and let it stand. +- Open questions go at the top or in their own section, never + mid-paragraph. + +## Calibration + +The two design documents in `docs/design_docs/` run 2,574 and 12,390 words, +both at 28 words per sentence, with no hedging phrases in the first and one +in the second. Sentence length is the thing to fix; aim for twenty words. +Do not import the hedging habit that these documents do not have. + +`docs/design_docs/template.rst` is the prescribed structure: a +`Requirement`, `Algorithm Design`, `Implementation` and `Testing` section +per topic, with the same topic name used in each. It also says requirements +"should not discuss technical software issues, but rather focus on model +capability", which is the rule most often broken. + +## Enough + +From `docs/design_docs/cached_outputs.rst`. The heading states the +requirement and the body is one or two normative sentences. + +> Requirement: updating cached outputs +> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> +> There should be a documented process for creating cached outputs for steps +> and uploading them. +> +> Requirement: either "normal" or "cached" versions of a step +> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> +> We **do not** require the ability to set up a "normal" and a "cached" +> version of the same step within a ``compass`` test case or suite. + +Five requirements in that document take about 250 words between them, and +saying which capability is *not* required is a requirement worth writing. diff --git a/.claude/skills/plan-documents/SKILL.md b/.claude/skills/plan-documents/SKILL.md new file mode 100644 index 0000000000..0e4184576b --- /dev/null +++ b/.claude/skills/plan-documents/SKILL.md @@ -0,0 +1,28 @@ +--- +name: plan-documents +description: Write a plan for work not yet started, for a colleague or the user to approve before implementation begins. +--- + +# Plan documents + +The reader is deciding whether to let you proceed. + +- Open questions and anything needing a decision go at the top. +- The steps, in order, one line each. +- Do not justify each step. Do not list the files you will touch. Do not + restate the codebase back. +- If a step needs a paragraph to explain, it belongs in a design document. + +## Enough + +Plans are approved in conversation rather than committed, so there is no +human example in this repository to copy. The following is constructed. + +> **Open:** should the `anvil` rows come out of the machines table now, or +> wait until the config file is restored? +> +> 1. Fix the land-ice draft used for pressure in the thin-film cases. +> 2. Restore the culled thin-film region in `initial_state`. +> 3. Take the smoothing from ice thickness, not draft, in both cases. +> 4. Update the `isomip_plus` docs in both guides. +> 5. Rerun the `pr` suite on Chrysalis against a `main` baseline. diff --git a/.claude/skills/pr-descriptions/SKILL.md b/.claude/skills/pr-descriptions/SKILL.md new file mode 100644 index 0000000000..4d9329c838 --- /dev/null +++ b/.claude/skills/pr-descriptions/SKILL.md @@ -0,0 +1,72 @@ +--- +name: pr-descriptions +description: Write or update a pull request description, including drafting pr_description.md before opening a PR. Use when opening a pull request or editing its body. +--- + +# Pull request descriptions + +The reader is deciding whether to review. + +- What changed and why, in a few sentences. Not how. +- Anything needing a reviewer decision goes in its own short list near the + top, never mid-paragraph. +- A list of changed behaviours is fine. A trace of the mechanism is not. +- No commit list. No testing; that goes in a separate `Testing` comment. +- Link the issue or upstream pull request that gives context. +- Several fixes usually means several pull requests. + +## Calibration + +Measured over merged pull requests from 2023 and 2024, before any agent +wrote here, excluding bots. Descriptions run 32 to 36 words at the median, +57 to 60 at the seventy-fifth percentile, 94 to 106 at the ninetieth, and +230 at the longest. Roughly one description in ten is empty; a title that +says it all is an acceptable description. + +## Enough + +A bug fix, stated and done, from #609: + +> The code for changing permissions was being given an incorrect directory +> and then was failing silently. This merge fixes the path and also removes +> the silent failure (an error will be raised if the status of the download +> directory cannot be determined). +> +> This merge also adds the database root and the root for each affected core +> to the list of directories to chmod/chown anytime any files get downloaded. +> +> closes #586 +> closes #608 + +A port, in one sentence, from #519: + +> This PR moves the generation of an initial state for the ocean global +> cosine bell test case from init mode to the initial state compass step. + +A change with a caveat the reviewer needs, from #716: + +> This PR replaces the `mode_init` MPAS-Ocean run in the `initial_state` +> step with local computations. The initial state is unchanged for the +> configuration that is currently used in the drying_slope cases. I did not +> retain all of the configuration options that were previously present in +> `mode_init`, e.g., the idealized transect. +> +> I added some buffer space in the y-dimension (the new config options `Ly` +> as differentiated from `Ly_analysis`) because the 1km case did not include +> the full 25km stretch of wetting and drying. + +## Too much + +Constructed, in the style of a real agent-written description elsewhere. +Five fixes each got a section, and each section traced its mechanism: + +> `compass.ocean.tests.global_ocean` gained a module-scope import of +> `compass.ocean.mesh.remap_topography`, which runs +> `compass/ocean/mesh/__init__.py`, which imports the step classes, which +> import `compass.ocean.tests.global_ocean` back. The module could no longer +> be imported on its own; it worked only when something else imported +> `compass.ocean.mesh` first. + +Someone deciding whether to review does not need the cycle traced. One +sentence would do; the rest belongs in the commit message. Five fixes is +also five pull requests. diff --git a/.claude/skills/review-comments/SKILL.md b/.claude/skills/review-comments/SKILL.md new file mode 100644 index 0000000000..6ae2423c16 --- /dev/null +++ b/.claude/skills/review-comments/SKILL.md @@ -0,0 +1,74 @@ +--- +name: review-comments +description: Write a review comment, review findings, or a reply to review feedback on a GitHub pull request. Use when reviewing code, reporting what testing someone else's branch turned up, or answering a reviewer's question. +--- + +# Review comments + +The reader is deciding what to change. + +- Put each finding as an inline comment on the line it concerns, one point + each. That is where colleagues put them, and it is why their review + bodies are short. +- The review body summarizes: what you ran, and the verdict. Two or three + sentences. +- Use a list in the body only for requests that span files. +- No section on what already works. One line for all of it, if any. +- Say what you could not check. + +## Calibration + +Measured over review comments from 2023 and 2024, before any agent wrote +here. Review bodies run 21 to 23 words at the median, 80 to 92 at the +ninetieth percentile, and 383 at the longest. Inline comments run 14 words +at the median, 58 at the ninetieth percentile, and 517 at the longest. + +## Enough + +Inline, one point and a suggestion, from #547 and #738: + +> I'd be inclined to call this test group `BaroclinicGyre` rather than +> `MitgcmBaroclinicGyre`. I think shorter is better, and I doubt we'll be +> implementing multiple baroclinic gyre cases. + +> Ok. Why don't you leave a comment that mentions that they refer to the old +> METIS partitioning so others know why they are there? + +Inline, a question rather than an assertion, from #817: + +> Is the tilde correct here? Won't this overwrite all the good data and +> leave the missing/bad data in place? + +A body that is the verdict and nothing else, from #523: + +> Thanks for doing this, @xylar. I'm approving based on my testing on Cori, +> in which I built spack with Albany and successfully ran the MALI +> `full_integration` test suite. I also had a quick look through the code +> diffs and nothing jumped out at me, although I can't pretend that I +> understand all of it. + +In the body, when several requests span the whole change, from #637: + +> @scalandr, this is excellent! +> +> I have some small changes and then there are a few other things to do: +> * the test group and all its classes and methods need to be added to the +> `ocean/api.rst` in the developer's guide +> * It would be great if you could build the documentation locally (see +> checklist above) to make sure it looks right. +> * Document the testing you did in a comment in the PR + +## Too much + +Constructed, in the style of a real agent-written review elsewhere, which +spent its first 444 words on "How this was reviewed", "What the previous +review asked for" and four paragraphs of "What works", then traced each +finding's mechanism: + +> The bin-centre array is static: computed once in the step's `setup()` from +> `nBins`, `minLat` and `maxLat`, and never updated. But it is added to the +> output stream like any other field, so every output file in every run +> carries a copy of the same 61 numbers. + +The finding is that a static array is written to every output file. Say +that inline, on the line that adds it, and stop. diff --git a/.claude/skills/testing-comments/SKILL.md b/.claude/skills/testing-comments/SKILL.md new file mode 100644 index 0000000000..4af60476ea --- /dev/null +++ b/.claude/skills/testing-comments/SKILL.md @@ -0,0 +1,82 @@ +--- +name: testing-comments +description: Write a Testing comment on a pull request, recording what was run and what the results were. Use after running suites, test cases or linting for a PR. +--- + +# Testing comments + +What you ran, where, and whether it passed. + +- Name the suite or test case, the machine and the compiler. One sentence. +- Give the work directory or the baseline you compared against. +- Say the result. Bit-for-bit, passed, or the numbers if they matter. +- Use a table or a list only when there are several runs to compare. +- Do not restate in prose what a table or a pasted result already shows. +- Failures unrelated to the branch go under their own heading at the end. + +## Calibration + +Measured over `Testing` comments from 2023 and 2024, before any agent wrote +here. They run 38 to 40 words at the median and 92 to 152 at the ninetieth +percentile. The shortest useful ones are under 25. + +## Enough + +From #663 and #816: + +> ## Testing +> +> I ran the `pr` suite on Chrysalis using `main` as a baseline and all tests +> passed with BFB results. + +> ## Testing +> +> I ran the `pr` suite with these changes, using the current `master` as a +> baseline. All tests passed. + +With the work directory, from #753: + +> ## Testing +> +> This mesh has been run through `files_for_e3sm` and the output is in: +> ``` +> /lcrc/group/e3sm/ac.xylar/compass_1.2/chrysalis/e3smv3-meshes/sowisc12to30e3r2 +> ``` +> +> I have verified that the land-ice mask includes "land-locked" cells that +> were formerly a problem for sea-ice, based on the fix in #752 + +Saying what you could not check, from #791: + +> ## Testing +> +> I successfully ran the `pr` and `nightly` suites on Chrysalis with Intel +> and Open-MPI. I didn't compare with a baseline because none is available +> for the Icos meshes. + +When the results are worth pasting, paste them and stop, from #771: + +> Tests are passing on Chrysalis (Intel/OpenMPI): +> ``` +> Test Runtimes: +> 06:22 PASS ocean_hurricane_DEQU120at30cr10rr2_mesh_fblts +> 46:58 PASS ocean_hurricane_DEQU120at30cr10rr2_init_fblts +> 06:21 PASS ocean_hurricane_DEQU120at30cr10rr2_sandy_fblts +> Total runtime 59:43 +> PASS: All passed successfully! +> ``` +> and the documentation looks good. + +## Too much + +Constructed, in the style of a real agent-written comment elsewhere, which +pasted the results table and then said the same thing again in prose: + +> Every test case now runs to completion. The five diffs are all of the form +> `File ... does not exist`: `main` crashed before writing those outputs, so +> there is nothing to compare against. Every comparison that had a file on +> both sides passed. A clean like-for-like comparison for those five cases +> needs a fresh baseline once this lands. + +The table already showed the passes and the five missing files. The only +new sentence is the last one. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000000..46580bb08e --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,151 @@ +# COMPASS Agent Instructions + +These instructions apply to the whole repository unless a deeper +`AGENTS.md` overrides them. + +## Source of truth + +- Follow the repo's automated style and lint configuration in + `pyproject.toml`, `.flake8.cfg` and `.pre-commit-config.yaml`. +- If an instruction here conflicts with automated tooling, follow the + automated tooling. + +## Environment + +- If `pixi-env/` exists, it is the preferred development environment for + Python, linting, and `pre-commit`. It is created by `./deploy.py`. +- AI agents should not run `./deploy.py` to create `pixi-env/` + themselves. Creating or refreshing `pixi-env/` is a developer action. +- Prefer running tools from `pixi-env/.pixi/envs/default/bin/` (for + example `python`, `pre-commit`, `flake8`, `isort`, and `mypy`) instead + of relying on the system environment. +- Only fall back to other Python environments if `pixi-env/` does not + exist or is clearly incomplete. + +## Python style + +- Keep Python lines at 79 characters or fewer whenever possible. +- Follow `flake8` and `isort` as configured. Do not preserve manual + formatting or import ordering that `isort` would rewrite. +- Keep imports at module scope whenever possible. Avoid local imports + unless they are needed to prevent circular imports, defer expensive + dependencies, or avoid optional dependency failures. +- Avoid nested functions whenever possible. Prefer private module-level + helpers instead. +- Put public functions before private helper functions whenever + practical. +- Name private helper functions with a leading underscore when that fits + existing repo conventions. + +## Documentation + +- The documentation is reStructuredText built with Sphinx. Follow + `docs/developers_guide/docs.rst`, which is the authority on structure + and on the label conventions for cores, configurations and test cases. +- Every new core, configuration or test case needs documentation in both + the User's Guide and the Developer's Guide, in the same pull request as + the code. +- Add new or modified classes, methods and functions to the relevant + `api.rst` (`docs/developers_guide/api.rst` or the core's own, such as + `docs/developers_guide/ocean/api.rst`). +- Prefer starting from the nearest existing page for a similar test case + rather than writing a documentation page from scratch. + +## GitHub pull requests and issues + +- Do not hard-wrap. Write each paragraph and each bullet as a single + line, however long. GitHub wraps them for display, and hard breaks + make later edits show up as reflowed paragraphs in the diff. +- Start with a paragraph summarizing what the pull request or issue is + about, then use sections for the detail. +- Keep the description in a file at the root of the worktree for the + branch it describes, and never commit it. It is a draft to paste into + GitHub, not part of the branch's content. +- Follow `.github/pull_request_template.md`: the description goes at + the top, keep only the checklist lines that apply, and use closing + keywords for any issue the pull request fixes. +- Do not list individual commits in a pull request description. The + commits are already on the pull request; describe what the change + accomplishes as a whole instead. +- Do not describe testing in a pull request description. Testing goes + in its own `Testing` comment on the pull request, which is what the + template's checklist asks for. +- An issue should say what happens, what was expected instead, and + enough about the configuration and commands used to reproduce it. + +## Writing for human readers + +These rules apply to anything a colleague reads: GitHub comments, pull +request descriptions, issues, plans, design notes. Not code comments or +commit messages, where a reader who wants the mechanism is already in the +right place. Per-artifact rules and worked examples are in +`.claude/skills//SKILL.md`, as plain markdown. Claude Code loads +the matching one automatically; other agents should read it before writing. + +Write less; do not pack the same content into denser sentences. Keep +headings, tables and links. Colleagues mostly write unstructured prose, and +structure is an improvement on it. The problem is length. + +- **Lead with the answer.** The first two sentences say what you found, + changed, or propose. Setup and reproduction go last. +- **One point per paragraph, and few paragraphs.** Colleagues write one to + three per comment; recent AI-written ones ran to eighteen. That gap is + the complaint. Say each thing once. +- **Do not narrate the mechanism.** The chain of calls, and why the fix is + right, go in the commit message. Here, say what broke and where to look. +- **Cut clauses that qualify rather than inform**, and any sentence whose + only job is to justify the one before it. One clause per sentence where + one will do. +- **Use backticks about half as often as feels natural.** They are for what + a reader would type or grep. Code blocks hold artifacts you did not + write, never authored prose. +- **One document, one decision.** Anything still relevant after this merges + is an issue, not a comment. + +Sign anything posted to GitHub on someone's behalf: + +``` +--- + +*Posted by on @'s behalf. The testing, analysis and wording +above are AI-authored; please check them accordingly.* +``` + +Name the agent, not the vendor: `Claude Code`, `Codex`, and so on. + +## Supported machines + +- The table under `Supported Machines` in + `docs/developers_guide/machines/index.rst` is the source of the + supported machine list in the Developer's Guide. Update it whenever + machines are added or removed, or compilers and MPI libraries are + added, removed, or renamed. +- Keep it consistent with the machine config files in + `compass/machines/`: the `mpi_` options under `[deploy]` + define the valid compiler and MPI combinations, and the + `__target` options under `[build]`, where present, + define the MPAS make targets. +- Keep `deploy/albany_supported.txt` consistent with the machines and + compilers that actually support Albany, since MALI functionality + depends on it. +- When a compiler is added or renamed, update every place it appears: + the machine config file in `compass/machines/`, the machine pages in + both the User's and Developer's Guides, and any `load_compass_*.sh` + examples in the documentation and tutorials. + +## Contracts + +- Treat `deploy.py` and `deploy/cli_spec.json` as contract files shared + with the `mache` package. +- Do not modify `deploy.py` or `deploy/cli_spec.json` directly in + Compass. +- If a change appears necessary, stop and note that the change must be + made in `mache` first, then synced back into Compass using the normal + upstream update process. + +## Validation + +- Run pre-commit on changed files is required before finishing; if sandboxed + execution fails, request escalation and do not close the task until it has + run or the user declines. +- Prefer fixing lint and formatting issues rather than suppressing them. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..eaf20eec4e --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +See [AGENTS.md](AGENTS.md) for all repository instructions.