Skip to content

Rewrite the Copilot instructions as PR-review guidance #310

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.

Sign up for GitHub

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

Merged
kandrosov merged 2 commits into from
Aug 28, 2026
Merged

Rewrite the Copilot instructions as PR-review guidance #310

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9e53e0
rewrite the Copilot instructions as PR-review guidance
kandrosov Aug 27, 2026
ef5345a
require documentation to ship in the same PR
kandrosov Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
306 changes: 194 additions & 112 deletions .github/copilot-instructions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,112 +1,194 @@
# FLAF - Copilot Instructions

## Overview

**FLAF** (Flexible LAW-based Analysis Framework) is a CMS (CERN) high-energy physics analysis framework designed to support multiple analyses. The project uses [LAW](https://github.com/riga/law) (Luigi Analysis Workflow) for task management and is designed to run on CERN computing infrastructure (lxplus, HTCondor, CVMFS).

- **Repository size**: ~2 MB
- **Languages**: Python (~13,000 lines), C++ headers, Bash scripts, YAML configs
- **Target environment**: CERN lxplus (AlmaLinux 9), requires CVMFS and CMSSW
- **Submodules**: `PlotKit` (must be cloned with `--recursive`)
- **Vendored**: `RunKit` (subset of workflow utilities, integrated directly — no longer a submodule)

## Project Structure

```
FLAF/
├── .github/ # CI workflows and GitHub Actions
│ ├── workflows/ # formatting-check, repo-sanity-checks, ds-consistency-check, trigger-flaf-integration
│ └── integration_cfg.yaml # Integration test configuration
├── Analysis/ # Analysis task implementations (histograms, merging, plotting)
├── AnaProd/ # AnaTuple production tasks
├── Common/ # Shared utilities (Setup.py, Utilities.py, etc.)
├── config/ # Era-specific configs (Run2_*, Run3_*), cross-sections
├── docs/ # MkDocs documentation
├── include/ # C++ headers for ROOT analysis
├── run_tools/ # Helper scripts (mk_flaf_env.sh, law_customizations.py)
├── test/ # Test scripts (checkDatasetConfigConsistency.py)
├── RunKit/ # Vendored subset of RunKit workflow utilities
├── PlotKit/ # Git submodule for plotting
├── env.sh # Main environment setup script
├── bootstrap.sh # HTCondor bootstrap script
└── cmsEnv.sh # CMSSW environment wrapper
```

## Code Formatting Requirements (CI Enforced)

**All pull requests must pass formatting checks.** Always format code before committing:

### Python Files (`.py`)
```bash
pip install black
black --check --diff <file.py> # Check formatting
black <file.py> # Apply formatting
```

### YAML Files (`.yaml`, `.yml`, `.yamllint`)
```bash
pip install yamllint
yamllint -s -c .yamllint <file.yaml>
```
Key rules: 2-space indentation, spaces inside braces `{ }` and brackets `[ ]`, no document-start marker required.

### C++ Files (`.cpp`, `.h`, `.hpp`, `.cc`)
```bash
clang-format --dry-run --Werror --style "file:.clang-format" <file.cpp>
clang-format -i --style "file:.clang-format" <file.cpp> # Apply formatting
```

## GitHub Workflows (CI Checks)

These run automatically on PRs to `main`:

1. **formatting-check.yaml**: Validates Python (black), YAML (yamllint), C++ (clang-format) formatting
2. **repo-sanity-checks.yaml**: Checks for binary files (must use git LFS), calculates repo size delta
3. **ds-consistency-check.yaml**: Validates `config/*/samples.yaml` files with:
```bash
python3 test/checkDatasetConfigConsistency.py --exception config/dataset_exceptions.yaml Run3_2022 Run3_2022EE Run3_2023 Run3_2023BPix Run3_2024 Run3_2025
```
4. **trigger-flaf-integration.yaml**: Triggers GitLab integration pipeline via `@cms-flaf-bot test` comments

## Key Configuration Files

| File | Purpose |
|------|---------|
| `.yamllint` | YAML linting rules |
| `.clang-format` | C++ formatting (Google-based, 120 column limit) |
| `.editorconfig` | Editor settings (4-space indent, UTF-8) |
| `config/law.cfg` | LAW task module registration |
| `config/<era>/samples.yaml` | Dataset definitions per era |
| `config/crossSections*.yaml` | Cross-section values |
| `mkdocs.yml` | Documentation site configuration |

## Important Notes for Code Changes

1. **Dataset configs** (`config/*/samples.yaml`): When modifying, ensure:
- MC samples have `crossSection` and `generator` fields
- `crossSection` values exist in `crossSections*.yaml`
- Run consistency check: `python3 test/checkDatasetConfigConsistency.py --exception config/dataset_exceptions.yaml <eras>`

2. **LAW tasks** (`AnaProd/tasks.py`, `Analysis/tasks.py`): Inherit from `Task` or `HTCondorWorkflow` in `run_tools/law_customizations.py`. After adding new tasks, run `law index --verbose`.

3. **Environment**: The framework requires CERN infrastructure (CVMFS, grid certificates). Local development needs `env.sh` sourced, which sets up a Python venv at `$ANALYSIS_SOFT_PATH/flaf_env/` using LCG_107_cuda. The `ANALYSIS_SOFT_PATH` environment variable (defaulting to `$ANALYSIS_PATH/soft`) is set automatically by `env.sh`.

4. **Binary files**: Never commit binary files directly. Use git LFS if needed.

5. **Git submodules**: `RunKit` and `PlotKit` are submodules. Python imports follow the pattern `from FLAF.RunKit.<module> import <function>` (e.g., `from FLAF.RunKit.run_tools import ps_call`).

## Validation Checklist

Before submitting changes:
- [ ] Run `black --check` on modified Python files
- [ ] Run `yamllint -s -c .yamllint` on modified YAML files
- [ ] Run `clang-format --dry-run --Werror` on modified C++ files
- [ ] If modifying `config/*/samples.yaml`, run dataset consistency check
- [ ] Ensure no binary files are staged (`git status` should show only text files)

## Trust These Instructions

These instructions are validated. Only search the codebase if:
- Information appears incomplete or outdated
- A command fails unexpectedly
- The task requires understanding code not covered here
# FLAF — instructions for Copilot code review

FLAF is the shared framework behind the CMS analyses HH_bbtautau, HH_bbWW and H_mumu. It builds
[LAW](https://github.com/riga/law)/luigi task graphs that run on HTCondor and CRAB, reads and
writes multi-TB datasets over GFAL, and JIT-compiles C++ into RDataFrame.

**A change here reaches every analysis and, through them, productions that take days of grid
time.** The failures that matter are silent ones: a job that exits 0 having written nothing, a
histogram normalised by the wrong denominator, a task that reports "complete" because a stale
path exists. Those cost days. A misplaced import costs seconds.

## What a useful review comment looks like here

Prioritise, in order:

1. **Silent wrongness** — a code path that produces a plausible but incorrect number, or reports
success without doing the work. Say what input triggers it and what the wrong output is.
2. **Violations of the framework invariants below.** They are not deducible from the diff; they
are why this file exists.
3. **Concurrency and remote-storage assumptions** — shared state under `law --workers`, ordering
between tasks, anything assuming a remote write is immediately visible.
4. **Genuine logic errors** — off-by-one, wrong branch, mishandled empty input.
5. **Documentation that did not ship with the change** — see the section below; a user-visible
change with no documentation update is an incomplete PR, not a nitpick.

Anchor a comment to a concrete failure: *"with `--workflow local` this also forces
`AnaTupleFileTask` local, so 10k branches run on the submit node"* is actionable. *"consider
adding error handling"* is not.

If the diff is fine, say so briefly. Volume is not value: three real findings beat thirty
observations.

## Framework invariants

Each of these has caused a production incident. They are ordered by how much damage they do.

### law semantics

- **`workflow` is a *significant* luigi parameter and `req()` copies it.** A task pinned to
`workflow="local"` (e.g. because its output is a `local_target`) therefore drags every task it
requires onto the local scheduler. Upstream workflow choice must travel on a separate
**insignificant** carrier parameter (`upstream_workflow` in `AnaProd/tasks.py`) that is
forwarded explicitly. It has to be a real `luigi.Parameter`, not an attribute, or branch and
workflow fall out of sync. Flag any new `req()` that forwards `workflow` implicitly.
- **`@law.dynamic_workflow_condition` objects are shared with subclasses.** A subclass that
decorates with the *parent's* `workflow_condition.output` mutates the shared object and
corrupts the parent task. It must `.copy()` first. Review the **parent** task too — the damage
shows up there, not in the subclass being changed.
- **law fixes the "already exists" branch set when luigi schedules a remote workflow**, before
its requirements run. A workflow whose own requirement creates its outputs will resubmit
everything. Clearing `_existing_branches`/`_skip_jobs` at the top of `run()` is the fix.
- **`poll()` snapshots the job count once.** Changing `job_data` mid-poll hangs or ends the loop
early, and a resumed run never calls `submit()` — hooks belong at the top of `poll()` too.

### Bundles (`run_tools/law_customizations.py`)

- **A bundle's output is a plain path, so law treats an existing one as complete forever.** Any
flavour packing code or configuration must be `hashed: true`, or jobs keep unpacking whatever
was built first and rebuild their branch map from *that* config — branch indices then mean
different datasets than the submitter intended.
- **A flavour must list every task whose output it packs** in `task_requires`. Miss one and the
tarball is built while that task is still writing. FLAF warns about a packed
`data/<version>/<Task>/<period>` directory that nothing requires; do not silence that warning.
- Bundles preserve symlinks inside a packed directory verbatim. An absolute symlink into AFS
therefore still sends every job back to AFS — which is what the bundle exists to avoid.

### Remote storage (`RunKit/law_gfal.py`)

- **`exists()` is answered from a cached directory listing, not a per-file stat.** Absence may
only be inferred from a *valid listing marker*. Never add a path that concludes "the directory
is known to exist, the file is not in what we have, therefore it is absent" — that reported
2260 of 11066 existing outputs as missing in production.
- The cache is two-level (in-process + a shared server). A change that makes every job do its own
`gfal-ls` will work in a test and DDoS the storage in production. Reviews should ask what a
change costs at 10k concurrent jobs.
- **Freshly written remote files can be invisible for seconds.** Code that writes and then
immediately checks must retry, not conclude absence.

### Processors and stitching

- **`stages` accepts only `AnaTuple` and `AnaTupleMerge`** — any other value is silently ignored.
A stitcher must appear at **both**: the first writes its denominator into the anaCache, the
second combines those caches. Present at `AnaTuple` alone, every merge of that process dies
with `combineAnaCaches: processor <name> not provided`. `dependency_level` is read only for
`AnaTupleMerge`.
- **Stitching variables must be readable at the merge stage.** Bins select on gen-level
quantities; an anaTuple that drops `GenPart`/`LHEPart` cannot evaluate them later. New bin
variables need the analysis to store them (`genInfo`) with a nanoAOD fallback.
- An empty stitching bin is not a bug: each bin's denominator is summed over the very events that
later read it, so a bin no event falls into is never divided by.

### Concurrency

- **Producers must not write bare-relative temp files.** CWD is shared between branches under
`law --workers`, so two branches race on the same name. Write under the job's working
directory.

## Configuration invariants

- `config_path_order` merges four directories: **scalars override, lists concatenate**. A list
added in an analysis config *extends* the framework one rather than replacing it.
- Dataset split: SM backgrounds and data live in `FLAF/config/<era>/datasets.yaml`; signals and
CI samples live in the analysis. A signal added to the framework config is misplaced.
- `Run3_2025` and `Run3_2026` carry no MC of their own — they set `reuse_mc_from_era: Run3_2024`.
A dataset list edited for 2024 changes all three.
- Cross-section keys referenced by a dataset must exist in `crossSections*.yaml`; CI checks this,
so flag it only when the diff adds a reference CI cannot see.

## Testing expectations

Physics correctness cannot be unit-tested without CERN infrastructure, but the framework's
mechanics can, and `test/` has suites for the ones that bit us (path cache, bundle hashing,
stitching variables, cost model). Changes to those areas should extend them.

When a test uses a fake, the fake must call the **real** `__init__` and patch only what is
genuinely unavailable. Hand-mirroring a class's attributes creates a copy that silently stops
matching — that is how the path-cache suite went red for a whole merge cycle.

Note that CI runs only `test/test_setup_loading.py` (via the `test-setup-loading` workflow);
the pytest suites are not run anywhere, so a broken one is not caught automatically.

## Documentation must ship with the change

A PR must update the documentation **in the same PR** whenever it changes anything a user of the
framework can observe. Treat this as a review item of the same weight as correctness — docs
drifting from the code is the failure that motivated the current documentation, and a PR that
lands without them is not complete.

Ask, for every diff: does it add, rename or remove any of these?

- a task or DAG node, or the arguments/parameters of one;
- a command, a CLI flag, or the meaning of an existing one;
- a configuration key — `global.yaml`, `user_custom.yaml`, `processes.yaml`, `phys_models.yaml`,
cross-sections, `fs_*` storage keys, bundle flavours, processor entries;
- a dataset, era, process or physics-model name;
- the environment, installation or setup steps;
- storage locations, output paths or log locations;
- a CI workflow, or how the integration test is triggered or configured;
- any behaviour a user relies on, including a default that changes.

If the answer is yes and the diff touches **no** documentation file, say so and name the page that
should have changed. If the author states the change is internal-only, that is a legitimate
answer — a pure refactor or bugfix with no user-visible effect is exempt — but it should be
stated in the PR, not left implicit.

Also flag the inverse: documentation edited to describe behaviour the diff does not implement, and
new pages added without being wired into `mkdocs.yml`'s `nav` (the build fails on that, but the
review should catch it first).

Where it goes in this repository:

- `docs/` is the source of truth for framework-wide material. Keep generic content here and link
to it from the analyses rather than duplicating it.
- `docs/reference/tasks.md`, `docs/workflow/arguments.md` and `docs/configuration/*` are the pages
that go stale first, because they enumerate tasks, arguments and config keys.
- New pages must be added to `nav:` in `mkdocs.yml`.
- Verified with `mkdocs build --strict`, which fails on a broken link, a missing nav entry or a
missing asset.

An ecosystem-level change — a new analysis, a new site, a renamed entry point — also needs the
landing page in `cms-flaf/cms-flaf.github.io`, which is a separate repository and therefore a
separate PR; say so in the review rather than assuming it will be noticed.

## Already enforced by CI — do not comment on these

`formatting-check` (black, yamllint, clang-format), `repo-sanity-checks` (binary files, repo size),
`ds-consistency-check`, `cross-section-check`, `test-setup-loading` (loads `Setup` for all seven
Run 3 eras). Formatting, indentation, quote style and trailing whitespace are settled by tooling;
comments about them are pure noise.

## Do not flag

- **Comment density or missing docstrings.** House policy is comments only where the *why* is
non-obvious; do not ask for narration of what the code already says.
- **PyROOT idioms** — C++ passed as strings to `ROOT.gInterpreter.Declare()` / RDataFrame
`Define()`, `from FLAF.Common.HistHelper import *`. These are deliberate.
- **Per-era config duplication.** Eras are kept explicit on purpose; "factor this out" is wrong.
- **Requests for unit tests of code that needs CVMFS, a grid proxy, or real NanoAOD.**
- **Broad refactors** of code the diff merely touches.
- **Speculative hardening** with no failure mode behind it.

## Repository facts

Verified 2026-08-27; re-check before relying on any of it.

| | |
|---|---|
| Layout | `AnaProd/` (anaTuple production tasks), `Analysis/` (histogram/plot tasks), `Common/` (`Setup.py`, utilities), `Processors/` (stitching), `RunKit/` (vendored grid/job tools), `run_tools/` (`law_customizations.py`), `config/`, `include/` (C++ headers), `test/`, `docs/` |
| Submodule | `PlotKit` only. **`RunKit` is vendored**, not a submodule; imports are `from FLAF.RunKit.<module> import …` |
| Datasets | `config/<era>/datasets.yaml` for Run 3. Run 2 eras still use the older `samples.yaml` |
| Eras | `Run3_2022`, `Run3_2022EE`, `Run3_2023`, `Run3_2023BPix`, `Run3_2024`, `Run3_2025`, `Run3_2026`; Run 2 legacy |
| Workflows | `formatting-check`, `repo-sanity-checks`, `ds-consistency-check`, `cross-section-check`, `test-setup-loading`, `deploy-docs`, `integration-test`, `trigger-flaf-integration` |
| Integration test | Triggered by `@cms-flaf-bot please test`. Its configuration (process lists, eras, versions) lives in **`cms-flaf/FLAF_ci`**, not in this repo |
| Docs | `docs/`, built with `mkdocs build --strict`; see the documentation section above |
Loading