[maxscheurer]

Closes #61

Replaces the flat parametrization string and parametrization_config with a required, nested per-component force-field spec (family: charmm | amber | openff plus protein / small_molecules / water_model overrides). Also fixes the SMIRNOFF parameter-cache key, locking and atom-type naming.

Stacked on #63 (base feature/issue-60-amber-sage-protein-parametrization), which is itself stacked on #59.

The implementation plan is posted as a comment below.

[maxscheurer]

Implementation Plan

Stack: develop ← PR 59 (feature/protein-mixedbox) ← PR 63 (feature/issue-60-amber-sage-protein-parametrization) ← this PR. The issue body says "base develop", but PR 63 is not merged yet and introduces AmberConfig, the Sage protein branch and the parametrization: run-schedule key, which are all reshaped here. This PR is therefore based on the PR 63 head branch and gets rebased when the lower layers change. Issue 62 (build-time mdp generation) should stack on top of this PR.

Decisions settled in planning (with the user)

1. Schema shape: a single nested parametrization field. A family discriminator names a coherent combination, and each family has per-component fields that can be overridden. A bare string is accepted as shorthand. parametrization_config is removed.
2. Defaults: there is no default anywhere, so parametrization (that is, the family) is required for every simulation type. Components inside a chosen family have family defaults.
3. SMIRNOFF cache key: force field + resolved charge method (the NAGL model file name or am1bcc/am1bccelf10, with the nagl alias resolved first) + an internal cache-format version. Water is keyed on the water model and ions on the force field. Installed openff-toolkit/interchange versions are written into the cache directory as provenance but are not part of the key.
4. Protein-preparation options: ignore_hydrogens and merge_all move to system.protein (ProteinSpecies), next to chains, disulfide_bonds and protonation_states.

Problem analysis

• parametrization is a flat string that mixes small-molecule force fields (cgenff, smirnoff), a tool (pdb2gmx) and families (charmm, amber). parametrization_config repeats the same value as a discriminator. The force field each component gets is implicit in branching code: isinstance on config classes in build.py, and getattr(par, "parametrization", "cgenff") in parametrize.py.
• The cgenff default is invalid for both protein types.
• BuildInput uses pydantic's default extra="ignore". After a schema change, an old parametrization_config key would therefore be silently dropped unless it is rejected explicitly.
• SMIRNOFF cache (parametrize.py):
  • Entries live in smirnoff/<ff-stem>/<species.hash>/. Water lives in <ff-stem>/water/ but is generated from ForceField(water_model).
  • charge_method and water_model are not part of the key.
  • Validity is checked with a bare itp_path.is_file(), and a half-written directory blocks retries because of mkdir(exist_ok=False).
• Locking: retrieve_or_deposit_parameters locks parameter_store/<engine>/<parametrization>/<hash>, which is not the directory SMIRNOFF writes to. Some callers also bypass it entirely, for example the additional_species (ions) calls in the mixedbox, bilayer and LNP builders.
• Atom-type names:
  • Interchange 0.5.5 names atom types f"{molecule.name}_{i}", and species.openff_molecule sets the molecule name to the resname. The names are therefore resname-derived and then cached by SMILES hash.
  • to_top has no public naming knob.
  • merge_extra_parameter_itps only deduplicates identical lines.
  • check_atomtype_collisions runs only on the protein path.

Target schema (deliverable contract)

parametrization:
  family: amber              # charmm | amber | openff (required)
  protein: amber14sb         # protein types only
  small_molecules:           # not allowed for proteinbox
    forcefield: openff-2.2.0.offxml
    charge_method: openff-gnn-am1bcc-0.1.0-rc.3.pt
  water_model: tip3p
# shorthand: `parametrization: amber` takes every component's family default

Family	protein	small_molecules	water_model	ions (derived, not chosen)	nonbonded scheme	allowed simulation types
charmm	charmm36m (proteinbox also accepts other non-LJ-PME CHARMM FFs, as today)	{forcefield: cgenff}	tip3p (proteinbox also accepts spc/spce, as today)	CHARMM ions from the bundled/CGenFF charmm36.ff	charmm (force-switch)	all five
amber	amber14sb	OpenFF Sage {forcefield, charge_method}	tip3p	Joung–Cheatham ions_tip3p.itp from amber14sb.ff	sage (potential-switch, DispCorr EnerPres)	proteinbox, protein_mixedbox
openff	n/a	OpenFF Sage {forcefield, charge_method}	opc3 (any OpenFF water name)	from the Sage .offxml	sage	mixedbox, bilayer, lnp

• water_model always means a water model name (tip3p, opc3, spce and so on). It maps to the pdb2gmx -water name, the native <name>.itp, or the OpenFF <name>.offxml, depending on the family. A value ending in .offxml is rejected with a hint.
• Validation happens per simulation type, with readable messages:
  • openff on a protein type is rejected, and so is amber on mixedbox/bilayer/lnp.
  • protein on a non-protein type is rejected, and so is small_molecules on proteinbox.
  • The existing CHARMM and Amber force-field/water restrictions stay, and the protein_mixedbox CHARMM bundle stays pinned to charmm36m/tip3p.
  • Components that do not apply to the type are stored as None, so metadata never shows a misleading component.
• The spec exposes read-only helpers for downstream code:
  • nonbonded_scheme, which issue 62 consumes.
  • A per-component description (protein, small molecules, water, ions) used in metadata, dry-run output and docs.
  • These are plain properties/methods, not computed_fields, so model_dump_json() → re-parse keeps working under extra="forbid".
• Old keys get explicit errors:
  • parametrization: cgenff|smirnoff|pdb2gmx → "use family: charmm / openff / charmm".
  • Any parametrization_config key → "moved into parametrization; prep options moved to system.protein".
  • The old type: discriminator and protein-prep fields inside parametrization → the same kind of hint.
  • This is needed for safety, not just convenience, because BuildInput ignores unknown keys.

Deliverables

1. Models (models/parametrization.py, models/input.py, models/species.py)
   • The three family specs, a discriminated union on family with string shorthand, the component sub-models, and the helpers listed above.
   • SmirnoffConfig, CgenffConfig, Pdb2gmxConfig, CharmmConfig, AmberConfig, _CharmmFamilyConfig and allowed_parametrizations-as-string-sets are removed. The CHARMM/Amber duplication is gone.
   • BuildInput.parametrization is required and parametrization_config is removed. Defaults that depend on the simulation type are resolved, and invalid components are rejected, in a BuildInput validator.
   • ProteinSpecies gains ignore_hydrogens (default True) and merge_all (default False). validate_protein_chain_config reads them from system.protein.
   • GromacsSingleMoleculeParameterSet records the component (small_molecule/water/ion), the force field that produced it, the charge method for OpenFF solutes, and an explicit topology-merge kind (cgenff/smirnoff/native).
   • The loose parametrization_config: ParametrizationConfig field on the parameter set goes away. Native TIP3P sets therefore no longer carry an opc3.offxml config, which resolves the PR 63 review note.
2. Dispatch (build.py, parametrize.py)
   • One resolver turns the spec (plus the optional native force-field directory) into the solute parametrizer. It replaces _get_parametrize_function, the SMIRNOFF-only partial, and _protein_solution_parametrizer.
   • All five builders use the resolver, and the LNP core/shell temporary inputs pass the spec through.
   • DISPATCH_ENGINE_PARAMETRIZE is re-keyed by small-molecule force-field kind (cgenff, openff), so the extension point documented in adding-parametrization.mdx survives.
   • run_pdb2gmx/_prepare_protein take the protein force field, the water model and the prep options from their new homes.
   • The isinstance checks against config classes are replaced by checks on the family.
   • generate_gromacs_topology_with_protein selects its merge branch from the parameter set's explicit merge kind. The getattr(..., "cgenff") fallback and the scheme_label string control flow are removed.
3. SMIRNOFF cache
   • A new layout root, distinct from the old gromacs/smirnoff/<ff-stem>/…, so old entries are never read. Solutes are keyed on force field + resolved charge method + format version, water on the water model, and ions on the force field.
   • Directory names stay readable (a force-field stem plus a short digest).
   • A provenance file records the key inputs and the installed toolkit/interchange versions.
   • An entry counts as cached only when all expected artifacts exist and a completion marker was written last. An incomplete directory is cleared and regenerated under the lock, so the exist_ok=False dead end is gone.
4. Locking
   • Each parametrizer locks exactly the directory it writes, using the same path computation. This covers CGenFF solutes, OpenFF solutes, OpenFF water and OpenFF ions.
   • retrieve_or_deposit_parameters is removed (or reduced to a pass-through that nothing uses). Every builder call path, including additional_species, is therefore covered.
   • The native water/ion short-circuit needs no lock.
   • The CGenFF cache path gromacs/cgenff/<hash> is left unchanged on purpose, so existing licensed CGenFF results stay valid.
5. Atom-type naming
   • After Interchange export, the atom types of every OpenFF-generated parameter set are renamed in _atomtypes.itp, in the [ atoms ] type column of the ITP and in the derived _params.itp. This covers solutes, water and ions.
   • The new names have the form <prefix>_<i>. The prefix is derived deterministically from the parameter-set identity (species hash + cache key), contains only ASCII letters, digits and underscores, starts with a letter, and is short (≤ 16 characters in total).
   • Atom names, topology_atom_names and the PR 63 unique-atom-name copies are untouched.
   • check_atomtype_collisions also runs in generate_gromacs_topology (mixedbox/bilayer/lnp) for OpenFF sets, and stays as a guard on the protein path.
6. Run schedules (run_schedules/__init__.py, run_schedules.yaml)
   • The optional per-entry selector key parametrization: is renamed to nonbonded_scheme: (sage for the existing proteinbox_amber entries).
   • RunScheduleManager selects by nonbonded_scheme. An entry with a matching key wins; otherwise the generic entry is used, as today.
   • _finalize_protein_build passes inp.parametrization.nonbonded_scheme.
   • Existing mdp files are unchanged. FF-dependent mdps for mixedbox/bilayer/lnp remain issue 62.
7. Persistence, CLI and display
   • BuildInput.metadata/to_data_row, flatten_system_parameters, check_run_exists, the utils/push.py placeholder row ("cgenff" → a valid family), the utils/pull.py docstring and filter, the orchestration/build.py dry-run line, and the cli.py --parametrization help (charmm/amber/openff) all use the family name.
   • The full component spec stays in input_data JSON.
   • RUN_DATABASE_COLUMNS/SUMMARY_COLUMNS keep the parametrization column. No schema migration of the remote DB is needed.
   • The dry-run line and metadata also show the per-component description.
8. CSV
   • Dotted columns: parametrization (shorthand) or parametrization.family plus parametrization.<component>[.<field>], and system.protein.ignore_hydrogens / system.protein.merge_all.
   • prepare.py probably needs no change beyond confirming that these columns round-trip.
9. Docs and examples
   • Docs pages: user-guide/parametrization.mdx (rewritten around the families/components table, the corrected cache layout and key, and the atom-type naming), yaml-format.mdx, csv-format.mdx, system-types.mdx, quick-start.mdx, installation.mdx, license.mdx, tutorial-mixedbox-campaign.mdx, cli-reference.mdx, sync.mdx, developer-guide/adding-parametrization.mdx, extending-engines.mdx and custom-builders.mdx.
   • Also README.md and SKILLS.md.
   • All 16 example YAML/CSV files under examples/.
   • A short "migrating from the old keys" note, including that old build directories, run-DB rows and parameter_store/gromacs/smirnoff/… entries are not reused (clean break).

Acceptance criteria (mapped to the issue)

• The schema describes each component (protein, small molecules, water, ions) by force field, not by tool. CharmmConfig/Pdb2gmxConfig and their shared base are gone. The protein-prep options live on system.protein.
• parametrization is required for every simulation type. Omitting it, or using an old value or key, gives a readable ValidationError. No invalid default exists.
• Each existing route is expressible as a family plus overrides, and builds as before:
  • CGenFF and SMIRNOFF for mixedbox/bilayer/lnp are charmm and openff.
  • CHARMM36m for proteinbox/protein_mixedbox is charmm.
  • Amber/Sage for proteinbox/protein_mixedbox is amber.
• "Builds as before" means generated topologies differ only in the intended ways (renamed OpenFF atom types, new cache paths), and the opt-in real-GROMACS test plus the cluster e2e runs pass.
• The run database, the pull --parametrization filter, check_run_exists, analysis metadata (flatten_system_parameters), the dry-run output and the CLI help all use the family name. Hash changes are accepted.
• Changing charge_method (including nagl versus an explicit model file, which resolve to the same key) or water_model never reuses a stale entry. Incomplete entries are regenerated. The locked path equals the written directory for every parametrizer.
• Two different molecules with the same resname, parametrized in separate calls, combine in one mixedbox and one protein_mixedbox topology without conflicting [ atomtypes ]. OpenFF atom-type names no longer contain the resname.
• Parameter sets record component, force field and (for OpenFF solutes) charge method. Native Amber water/ion sets report amber14sb/tip3p.
• nonbonded_scheme is exposed per family and selects the proteinbox_amber mdps for Amber builds. CHARMM and other builds get exactly the files they get today.
• Docs, README.md, SKILLS.md and every example YAML/CSV describe the new schema, including the protein types. Every example file parses into a BuildInput (enforced by a test).

Files to create or modify

• mdfactory/models/parametrization.py: family specs, component models, helpers, parameter-set fields. Old configs removed.
• mdfactory/models/input.py: required nested parametrization, per-type resolution/validation, old-key errors, prep options read from system.protein.
• mdfactory/models/species.py: ProteinSpecies.ignore_hydrogens / merge_all.
• mdfactory/parametrize.py: resolver/dispatch, cache layout/key/marker/provenance, per-directory locking, atom-type renaming, explicit merge kind, collision check in generate_gromacs_topology.
• mdfactory/utils/topology_utilities.py: atom-type renaming helper (ITP [ atoms ] type column + [ atomtypes ]).
• mdfactory/build.py: all five builders, _prepare_protein, _finalize_protein_build, LNP temporary inputs.
• mdfactory/setup/protein.py: run_pdb2gmx signature (protein FF, water, prep options).
• mdfactory/run_schedules/__init__.py, run_schedules.yaml: nonbonded_scheme selector.
• mdfactory/analysis/utils.py, mdfactory/utils/pull.py, mdfactory/utils/push.py, mdfactory/utils/data_manager.py, mdfactory/orchestration/build.py, mdfactory/cli.py: family name and help text. analysis/constants.py is only checked, since the column names are unchanged.
• Tests (see below), docs, README.md, SKILLS.md, examples/**.

Testing approach

New tests are named without "build" so they run under pytest -k "not build" mdfactory, unless they really build.

• Schema (test_models.py, or a new test_parametrization_schema.py):
  • Each family parses in both the shorthand and explicit form. The shorthand and the fully spelled default give the same hash.
  • Family component defaults are resolved per simulation type, and components that do not apply are None.
  • Every rejection rule from the table produces a readable message: family/type mismatch, a protein or small_molecules component where it does not apply, CHARMM and Amber FF/water restrictions, and an .offxml water name.
  • A missing parametrization is an error, and so is each old value/key (cgenff, smirnoff, pdb2gmx, parametrization_config, type, and prep options inside parametrization).
  • nonbonded_scheme and the component description per family.
  • The model_dump_json() → BuildInput(**json) round-trip.
  • merge_all/chains validation now on system.protein.
• Parametrization (test_parametrization.py, real OpenFF as in the existing SMIRNOFF tests, with parameter_store in tmp_path):
  • The cache directory differs when charge_method changes, and nagl and the explicit NAGL file share a key.
  • The water cache directory differs per water_model. Ions are keyed on the force field.
  • The provenance file contents.
  • A directory without the completion marker is regenerated instead of reused or raising FileExistsError.
  • The locked path equals the written path: monkeypatch lock_local_folder to record its argument, for a CGenFF-free OpenFF solute and for water/ions.
  • Atom types are prefix-based and deterministic, do not contain the resname, and are consistent between the ITP [ atoms ] column and _params.itp.
  • Parameter sets record component, force field and charge method.
  • Native Amber water/ion sets record amber14sb/tip3p.
• Collision regression (test_parametrization.py / test_protein_mixedbox.py):
  • Two different SMILES, both with resname LIG, parametrized in separate calls.
  • (a) generate_gromacs_topology produces extra_params.itp with no duplicate atom-type names carrying different parameters.
  • (b) generate_gromacs_topology_with_protein with a synthetic Amber-like FF dir (existing fixtures) passes check_atomtype_collisions.
  • A deliberately conflicting pair still raises in the non-protein path.
• Topology utilities (test_topology_utilities.py): the renaming helper on small synthetic ITP/atomtypes text, including comments, [ atoms ] lines with charge-group and mass columns, and a count mismatch that raises.
• Dispatch (test_protein_mixedbox.py, test_proteinbox.py, test_build.py with mocks as today):
  • The resolver returns the CGenFF or OpenFF parametrizer per family, binding the native FF dir for protein types.
  • run_pdb2gmx receives the protein FF, water and prep options from their new homes.
  • The merge branch is chosen by merge kind.
  • Mixing kinds raises.
• Run schedules (test_orchestration_mdp.py or the existing run-schedule tests):
  • sage on proteinbox/protein_mixedbox → proteinbox_amber.
  • charmm → generic proteinbox.
  • sage on mixedbox (no dedicated entry) → generic mixedbox.
  • An unknown scheme falls back the same way.
• Persistence/CLI:
  • test_pull.py: filter by family.
  • test_push.py: placeholder row is a valid family.
  • check_run_exists conditions.
  • test_analysis_utils.py: flatten.
  • test_orchestration_build.py: dry-run text.
  • test_simulation*.py: metadata.
  • test_orchestration_cli.py: CSV with dotted parametrization columns.
• CSV (test_prepare.py): parametrization shorthand, parametrization.family + parametrization.small_molecules.charge_method, and system.protein.merge_all round-trip into BuildInput.
• Examples: a new parametrized test loads every examples/**/*.yaml and *.csv into BuildInput. It only validates, without building, and uses a dummy pdb_path resolution if needed.
• Existing tests: update conftest.py fixtures and the ~18 files that use the old strings and configs to the new schema. No assertion is weakened.
• Real GROMACS / e2e:
  • Update the opt-in test_amber_gmx_build.py.
  • On the cluster (compute nodes, not the login node), rerun the PR 63 and PR 59 e2e matrix with the new YAMLs:
    • openff and charmm mixedbox/bilayer/lnp (CGenFF via the renewed SILCSBIO license).
    • charmm proteinbox/protein_mixedbox.
    • amber proteinbox/protein_mixedbox on cpu gmx and gpu gmx-cuda.
  • Build and EM/NVT must succeed, and results are posted as a PR comment.

Risks and open questions

• Blast radius: the string is read in more than 25 places, and a miss fails at runtime, not at import. Mitigations: grep for every old literal and config class as a final check, the examples-parse test, and the e2e matrix.
• CGenFF in CI: the CGenFF routes are skipped without SILCSBIO, so "builds as before" for charmm small-molecule builds relies on the cluster e2e.
• Clean break:
  • Old build directories, run-DB rows (input_data) and pushed data cannot be re-parsed into the new BuildInput. They fail with the old-key error.
  • The OpenFF cache is regenerated once, including NAGL/AM1BCC charges.
  • The docs say this explicitly; nothing is migrated.
• Atom-type renaming correctness:
  • The rename must hit every place a type appears: the [ atomtypes ] name, the [ atoms ] type column, and any typed sections Interchange might emit (it emits none for SMIRNOFF today, since bonded parameters are inline). That last point is verified on real output in tests.
  • OpenMM GromacsTopFile (used for the protein relax/compress stages) must still parse the renamed types. This is covered by the opt-in real-GROMACS test and e2e.
• Lock semantics: moving locks into the parametrizers changes the concurrency behaviour that test_parametrization_simultaneously exercises (a CGenFF test that is already flaky). It needs to stay green on the cluster.
• nonbonded_scheme naming (charmm, sage) is what issue 62 will build on. If issue 62's plan needs finer schemes, it extends the literal, without renaming.
• openff mixedbox/bilayer/lnp still use the generic (CHARMM-style) mdps, as today. This mismatch is pre-existing and belongs to issue 62. nonbonded_scheme makes it visible but does not change it.
• PR 24 (search/tagging, base main) also touches models/input.py, orchestration/build.py and parametrize.py, so a conflict is expected when the stacks meet.

Out of scope

• ff19SB/OPC/OPC3 (left for a follow-up).
• Build-time mdp generation and FF-dependent mdps for mixedbox/bilayer/lnp (issue 62).
• CGenFF cache-key versioning: the path is left unchanged, so existing licensed results stay valid.
• The CHARMM MET1 pdb2gmx bug.
• The issue 55 wheel force-include.
• The lnp mdp directory.
• The SLURM/simulate exit-code observations from PR 63.
• The PR 63 review nitpicks, except where rewriting the touched code resolves them anyway: the duplicated ion map, scheme_label control flow, and the opc3 metadata on native sets.

---

Plan created by mach6

[maxscheurer]

Progress Update

Implemented all plan deliverables in one commit.

• Schema: parametrization is required everywhere and is a family spec (charmm | amber | openff), given either as a bare family name or as {family, protein, small_molecules{forcefield, charge_method}, water_model}. BuildInput validation fills in family defaults per simulation type; components that don't apply are null. The family name alone and the fully spelled-out defaults give the same hash. parametrization_config and the old config classes are gone. ignore_hydrogens/merge_all moved to system.protein. The spec exposes nonbonded_scheme, components() and summary().
• Old keys: per the clean-cut decision there are no old-key-specific messages. BuildInput and the family specs now reject unknown keys (extra="forbid"), and an old value gives pydantic's union-tag error, so nothing is silently dropped. The migration table lives in the user guide.
• Dispatch: one resolve_parametrizer(spec, engine, native_forcefield) serves all five builders. DISPATCH_ENGINE_PARAMETRIZE is keyed cgenff/openff, and parametrize_smirnoff_gromacs is renamed parametrize_openff_gromacs. Parameter sets record component, forcefield, charge_method, water_model and topology_merge (cgenff/smirnoff/native). The protein topology merge branches on topology_merge. Native Amber sets report amber14sb/tip3p.
• OpenFF cache: entries live under gromacs/openff/{small_molecules,water,ions}/<stem>_<digest>/. Small molecules are keyed on force field + resolved charge method + format version, water on the water model, ions on the force field. Each entry has provenance.json (with toolkit/interchange versions) and a .complete marker; incomplete entries are regenerated under the lock. Each parametrizer locks exactly the directory it writes, and retrieve_or_deposit_parameters was removed. The CGenFF path is unchanged; a failed CGenFF attempt directory is now cleared and retried instead of raising FileExistsError.
• Atom types: OpenFF types are renamed to T<9 hex>_<i> in the atomtypes file, the ITP [ atoms ] column and _params.itp. The collision check also runs for mixedbox/bilayer/lnp.
• Run schedules: the selector key is nonbonded_scheme (sage for the proteinbox_amber entries).
• Persistence/CLI: the DB row, metadata, check_run_exists, flatten_system_parameters, the dry-run line (now also shows components), the push placeholder and the --parametrization help all use the family name.
• Docs/examples: the parametrization guide and the developer guide are rewritten, other pages, README and SKILLS are updated, and all 16 examples are converted. A new test parses every example YAML/CSV.
• Removed the charmm_native_atom_names alias.

Tests: pytest -k "not build" mdfactory: 3292 passed, 1 skipped. New: test_parametrization_schema.py, plus OpenFF cache key/marker/provenance/lock/atom-type tests and a same-resname collision regression (mixedbox and protein_mixedbox). Also new: rename-helper tests, dispatch/run-schedule fallback tests, and CSV dotted-column, pull/check_run_exists and dry-run tests.

Related pre-existing issues (not fixed): BuildInput.metadata fails for LNP (no system.species); docs/public/tutorials/mixedbox_chain_length_series_20.csv uses a 4-character resname (MEOH), which fails validation; conda's openff-interchange reports __version__ 0.0.0 (shows up in provenance).

E2E results — per-component parametrization specs (issue 61), 2026-11-05

Uncommitted working tree on top of d4ec008; jobs mdfactory_e2e/jobs/job61_{gpu,cpu,lnp,amber_sim}.sh. The example inputs are the converted repo examples/ (new schema), and the real parameter store is used (existing CGenFF entries reused, OpenFF entries regenerated under gromacs/openff/).

Family / system	Input	Env	Build	EM → NVT (100 ps)
charmm mixedbox (CGenFF)	water_ethanol_cgenff.yaml	dev + gmx_mpi 2024.3, cpu	OK	OK
openff mixedbox	water_ethanol_smirnoff.yaml	dev + gmx_mpi 2024.3, cpu	OK	OK
charmm bilayer (CGenFF)	popc_cgenff.yaml	dev, cpu	OK	OK
openff bilayer	popc_smirnoff.yaml	dev, cpu	OK	OK
charmm lnp (CGenFF)	lnp_alc0315.yaml	dev, gpu	OK	n/a (no lnp run schedule, pre-existing)
openff lnp (charge_method: nagl)	lnp_alc0315_smirnoff.yaml	dev, gpu	OK (627 unique T…_i atom types)	n/a
charmm proteinbox	lysozyme_charmm36m.yaml	dev + gmx_mpi 2024.3, gpu	OK	OK
charmm proteinbox CSV (lysozyme + 4-chain insulin)	proteins.csv	dev, cpu	OK (2/2)	—
charmm protein_mixedbox + ethanol (CGenFF)	lysozyme_fixed_box.yaml	dev + gmx_mpi 2024.3, gpu	OK	OK (generic Force-switch mdps)
amber proteinbox	lysozyme_amber14sb.yaml	gmx-cuda 2026.3, gpu	OK	OK (gmx env, cpu)
amber proteinbox CSV	proteins_amber.csv	gmx, cpu	OK	—
amber protein_mixedbox + ethanol (Sage)	lysozyme_amber_fixed_box.yaml	gmx-cuda, gpu	OK	OK (gmx, cpu; proteinbox_amber Potential-switch/EnerPres mdps)
amber protein_mixedbox water-only	lysozyme_amber_water_fixed_box.yaml	gmx-cuda, gpu	OK	OK (gmx, cpu)
opt-in real-GROMACS test	test_amber_gmx_build.py	dev + gmx 2026.3	passed	—

All protein builds passed the grompp -maxwarn 0 validation. OpenFF parameter sets carry T<9 hex>_<i> atom types (no resnames) in both the mixedbox and the protein_mixedbox topologies; the builds ran concurrently on three nodes against the same parameter store (per-directory locks).

Not related to this change: mdfactory simulate with the gmx-cuda env and the local executor aborts in mdrun on a GPU node ("setting the number of OpenMP threads without specifying the number of ranks", OMP_NUM_THREADS without -ntmpi); the same amber builds simulated fine with the CPU gmx env. PR 63 used --slurm for GPU runs.

Commit: 8298713

---

Progress tracked by mach6

[maxscheurer]

Unverified Review Candidates — Pending Assessment

Review round: 1
Reviewed commit: 8298713

These are unverified candidates. Severity reflects reviewer confidence; do not treat any item as a merge blocker until the assessment comment is posted.

Critical

None.

Important

1. lock_local_folder swallows KeyboardInterrupt/SystemExit (confidence 85, error-auditor). mdfactory/utils/utilities.py:233-258, now wrapping every CGenFF (parametrize.py:207-238) and OpenFF (parametrize.py:396-428) generation. except Exception does not catch BaseException; the finally unlinks the lock and returns, discarding the in-flight exception. Ctrl-C mid-parametrization falls through, so the build continues with an unfinished entry (possibly a returned parameter set pointing at an unmarked entry).
2. CGenFF cache has no completion marker (confidence 80, error-auditor). parametrize.py:207-238. Validity is itp_path.is_file() and prm_path.is_file() and files are written with plain open(..., "w"). A killed/failed write leaves a truncated file that later builds accept as a cache hit. The OpenFF side got .complete + regenerate.
3. Old-schema build directories silently skipped by analysis discovery (confidence 80, error-auditor). analysis/utils.py:85-92 (bare except Exception: continue) and analysis/simulation.py:727-741 (debug-level log, generic "No valid BuildInput YAML files found"). Pre-PR directories vanish from batch discovery without telling the user the YAML uses a removed schema.
4. parametrize_openff_gromacs never tested with a non-default force field / charge method (confidence 85, test-reviewer). The base-branch test_smirnoff_with_config (explicit openff-2.1.0.offxml + NAGL model, path asserted) was deleted with no end-to-end replacement. Non-default values are only tested on pure openff_*_cache_entry() helpers, so a parametrizer that builds ForceField/charges/cache entry from defaults instead of the spec would pass the suite and reintroduce the stale-cache bug.
5. Docs do not describe the protein system types (confidence 82, completeness-checker). Issue/plan criterion "docs … describe the new schema, including the protein system types". system-types.mdx has no proteinbox/protein_mixedbox section (and parametrization.mdx:65-66 links to it for system.protein); yaml-format.mdx:21 and csv-format.mdx (~l.18, ~l.107) list only mixedbox|bilayer|lnp; README.md:20-21 and SKILLS.md:3,11,248 mention only mixedbox/bilayer/LNP builders.

Suggestions

6. Stale .lock gives a non-actionable timeout error (confidence 80, error-auditor). utils/utilities.py:229-259. A hard-killed job leaves <workdir>.lock; later calls wait ~240 s then raise TimeoutError naming the folder, not the lock file or a "remove if stale" hint. Now exposed on all parametrization paths.
7. CSV: bare parametrization + dotted parametrization.* columns conflict silently/obscurely (confidence 80, error-auditor). prepare.py:65-108. Dotted-before-bare silently drops the override; bare-before-dotted raises an opaque TypeError.
8. Atom-type renaming not asserted for water/ions (confidence 80, test-reviewer). test_parametrization.py:527-565. test_openff_ions/test_openff_water check only disjointness/inequality; they pass if rename is skipped, and do not check ITP [ atoms ] vs _params.itp consistency.
9. Blank ignore_hydrogens/merge_all in a heterogeneous CSV untested (confidence 80, test-reviewer). test_prepare.py:196-210, prepare.py:20-30.
10. Unused SimulationType alias (confidence 95, simplifier) — models/parametrization.py:27.
11. Redundant conditional in _split_code_comment (confidence 95, simplifier) — utils/topology_utilities.py:660-664.
12. Duplicated section-scanning loop in rename_atomtypes (confidence 80, simplifier) — utils/topology_utilities.py:~698-745.
13. Duplicated cache-dir naming in the three openff_*_cache_entry functions (confidence 80, simplifier) — parametrize.py:~365-383.

Strengths

• Code reviewer found no defects ≥80: it exercised every family × simulation type, run-schedule selection, and real OpenFF parametrization (same-resname collision gives distinct T<hex>_<i> types, consistent between ITP and params).
• OpenFF cache design (key, .complete marker written last, sibling .lock, clear-and-regenerate under lock) audited with no silent-failure paths.
• Strong schema, key, marker, lock-path, collision (mixedbox + protein_mixedbox) and examples-parse test coverage.
• Retired config classes, parametrization_config and retrieve_or_deposit_parameters are gone from code, tests, docs and examples.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[maxscheurer]

Review Assessment

Findings comment: #65 (comment)

Round 1 (independent assessor only). Reviewed commit 829871312ca8c3b3ab9b1e6628eea81f74548da3.

Classifications

Finding	Classification	Reasoning
1. lock_local_folder swallows KeyboardInterrupt	useful follow-up	Factual: True and reproduced (utils/utilities.py:233-258: except Exception misses BaseException, return in finally discards it). Scope: Function is pre-existing (initial commit); the base's retrieve_or_deposit_parameters wrapped every CGenFF/SMIRNOFF call in the same lock with the same behaviour. PR only moves call sites and adds OpenFF ions. Practical: Needs interactive Ctrl-C (scancel/timeouts send SIGTERM/SIGKILL, which raise nothing). Cache is not poisoned: OpenFF entries without .complete are regenerated, CGenFF needs both files. Result is a confusing later FileNotFoundError.
2. CGenFF cache has no completion marker	deferred	Factual: True (parametrize.py:208,232-235), identical to base. Scope: Plan deliberately leaves the CGenFF cache path unchanged so existing licensed results stay valid; marker/regeneration criteria are under SMIRNOFF. PR improves the path (locked, failed attempt dirs cleared). Practical: Needs SIGKILL in the tiny window while two small strings are written; not a regression.
3. Old-schema dirs silently skipped in analysis discovery	useful follow-up	Factual: True (analysis/utils.py:85-89 bare continue; simulation.py:727-741 debug log + generic error); both pre-existing. Scope: Clean break approved; old dirs not re-parseable by design and documented in parametrization.mdx migration section. Practical: Accepted consequence; a warning log would help.
4. No end-to-end OpenFF test with non-default FF/charge method	useful follow-up	Factual: Partly true: non-default small-molecule values tested only at key-helper level; water (tip3p) and nagl are tested end to end. Current code uses the spec correctly (parametrize.py:546-566). Scope: Key-differs criterion met at key level; no criterion requires an e2e non-default run. Practical: Guards only against future hard-coding; nothing wrong today.
5. Docs lack protein system-type descriptions	useful follow-up	Factual: True: system-types.mdx has no protein section; yaml-format.mdx:21, csv-format.mdx:18,107, README.md:20, SKILLS.md:3,11,248 list only mixedbox/bilayer/lnp; parametrization.mdx:65-66 links to System Types for system.protein options that are not described there (new in this PR). Scope: The new schema is documented for protein types (families table, per-type rules, amber section, migration table, CSV columns) and all protein examples are converted and parse-tested; the missing system-type pages and enum lists date from PR 59. Core intent of the criterion is met. Practical: Low harm — same fields documented elsewhere. Cheap to fix (retarget the link, extend the simulation_type rows).
6. Stale .lock gives unhelpful timeout	deferred	Factual: True (utilities.py:259). Scope: Pre-existing; base already locked CGenFF/SMIRNOFF species and water. Practical: Needs a kill during seconds-long generation; error names the folder so <folder>.lock is discoverable.
7. CSV bare + dotted parametrization columns conflict	useful follow-up	Factual: True, reproduced (prepare.py:65-108; silent override loss in one order, TypeError in the other). Scope: No criterion covers mixing; generic dotted mapping pre-exists. Practical: Requires violating the documented "not both" rule (csv-format.mdx:127-128).
8. Atom-type rename not asserted for water/ions	nitpick	Factual: Water half is wrong (without rename both sets would carry SOL_*, failing test_parametrization.py:544); ions half true. Scope/Practical: All components share _ensure_openff_entry → rename_atomtypes (parametrize.py:411), which raises on undefined [ atoms ] types; small-molecule test asserts prefix and consistency.
9. Blank ignore_hydrogens/merge_all in CSV untested	nitpick	Factual: True. Practical: Same generic prefix mechanism tested for chains; a regression fails loudly (NaN error), not silently.
10. Unused SimulationType alias	nitpick	Factual: Confirmed dead. No behavioural effect.
11. Redundant conditional in _split_code_comment	nitpick	Factual: True; no behavioural effect.
12. Duplicated section-scanning loop in rename_atomtypes	nitpick	Style only.
13. Duplicated cache-dir naming	nitpick	Layouts differ intentionally (water has no key subdir).

Action Plan

No merge blockers. No action required before merge.

---

Assessment by mach6

[maxscheurer]

Progress Update

Fixed the CI failure on this PR. test_cgenff_locks_written_directory_and_regenerates_failed_attempt stubbed the CGenFF executable but resolved the native charmm36.ff/forcefield.itp from the licensed CGenFF installation, so it passed on machines with CGenFF and failed on the CI runner (path validation error on forcefield_itp). The test now builds a temporary charmm36.ff directory and passes it as native_forcefield. Test-only change; no product code touched.

Verified by simulating a missing CGenFF installation: the test failed before the change and passes after it. test_parametrization.py and test_protein_mixedbox.py pass locally (93 passed, 1 skipped), and ruff check and format are clean.

Commit: 571dedc

---

Progress tracked by mach6