Generator contract, shared evaluation layer, and HF-hosted leaderboard

[mkeeler43]

Generator contract, shared evaluation layer, and HF-hosted leaderboard

Replaces 13 near-duplicate evaluate_*.py scripts with one adapter per model
and a shared evaluation package, and makes HuggingFace the single home for
checkpoints, metrics, and the leaderboard.

Large, but split into four commits meant to be reviewed one at a time.
Commit 1 is a pure file move you can skim. The design fits in commit 2.

---

Why

Every model had a training script and an evaluation script. The evaluation
scripts were ~79% identical — evaluate_cgan_cnn_2d.py and
evaluate_gan_cnn_2d.py differed by 26 lines out of 123, and those 26 lines
were only ever four things: which network class to import, how to build it from
run config, whether to pass conditions, and a model-id string.

So adding a metric meant editing 13 files, and adding a model meant copying one.
A leaderboard can't be built on that.

Training scripts are deliberately untouched in structure. Their variation is
real — 340 to 1894 lines of genuinely different algorithms — so unifying them
would relocate complexity rather than concentrate it, and would cost the
single-file readability the repo is built around.

---

Review guide

1 · 646617d — Move model packages under engiopt/generators/ (68 files, skim)

Mirrors engibench/problems/<name>/. Pure move: file contents are unchanged
apart from the import paths the move forces (engiopt.<model> →
engiopt.generators.<model>). Git detects all 47 renames, so git log --follow
still works.

Nothing to review here beyond "yes, that's a move."

2 · 35f1041 — What defines a "generator" (5 files, read this one)

The whole design is here. Today a model is defined implicitly, by whatever its
evaluation script happened to do. This commit makes it explicit: a generator is
six facts and two actions.

The six facts a model states about itself. These are plain values, not code:

Fact	Example	What it buys you
algo_id	"gan_2d"	The name it appears under everywhere — its folder, its leaderboard row, its HuggingFace repo. One name, so those can never drift apart.
conditional	False	Whether it listens to the design requirements it is given. Unconditional models are still given them, so a metric can measure that they ignored them.
design_kinds	("2d",)	Which problems it can serve. Pointing a 2D model at a 3D problem now fails immediately with a clear message, instead of producing garbage.
checkpoint_files	("generator.pth",)	Which files must be present to load it. Missing files fail at load, not halfway through an evaluation.
primary_state_key	"generator"	Where the weights sit inside the saved file.
output_clip	(1e-3, 1.0)	The range its designs must stay in, because several simulators are unstable at exactly zero.

The two actions a model must implement:

• build — given a downloaded checkpoint, reconstruct me. The model
  receives the training run's saved settings and local file paths, and returns
  a working network.
• _sample — given design requirements, produce designs. This is the only
  genuinely model-specific step, and it is where models really differ: one line
  for a GAN, a 1000-step loop for diffusion, one forward pass per pixel for
  PixelCNN++.

Everything else is done once, centrally: finding and downloading the
checkpoint, picking CPU/GPU, choosing which hyperparameter configuration to
load, seeding for reproducibility, timing how long sampling took, reshaping
outputs, and clamping them to the valid range.

The outcome: a new model is ~45 lines, of which ~10 are unique to it. Before
this, adding a model meant copying a 123-line evaluation script and editing
four scattered places in it. The contract mirrors engibench.core.Problem, so
the two halves of the stack are learned once.

engiopt/utils/all_generators.py finds models by walking the folder, exactly as
engibench/utils/all_problems.py finds problems — BUILTIN_GENERATORS sits
alongside BUILTIN_PROBLEMS.

Checkpoint storage also changes here. Each training run is filed under a
short hash of its own hyperparameters:

{problem}/cfg_{fingerprint}/seed_{n}/     one per configuration
{problem}/seed_{n}/                        the "standard" one

The outcome: a 50-configuration sweep produces 50 separately loadable models
instead of 50 runs overwriting one folder. The plain {problem}/seed_{n} path
is claimed only by a run that used the training script's default settings —
so no amount of hyperparameter searching can change what a bare model name
means. W&B is no longer a checkpoint source at all.

3 · a796025 — Shared evaluation layer (28 files, −1942 lines)

Deletes the 13 eval scripts. Adds:

• engiopt/evaluation/ — a metric registry declaring each metric's family,
  cost, and ranking direction; an EvaluationContext that computes the
  expensive optimizer pass once for every performance metric (iog, cog,
  fog, viol share one sweep instead of running four); an Evaluator; and
  the leaderboard, published to HF by merging rather than overwriting.
• engiopt/specs/<problem>/v1.json — frozen, hash-verified evaluation
  contracts, so rows are comparable across people and across time.
• python -m engiopt.evaluate replaces the deleted scripts.
• engiopt/generators/_template/ — copy-me starting point for new models.

Metrics are registered functions, not model methods — a metric compares a
generated set against a reference set under a problem, so it belongs to the
comparison:

@register_metric("mmd", family="distribution", cost="cheap", higher_is_better=False)
def mmd(ctx) -> float: ...

Declared cost means a cheap run provably cannot start a simulator.

4 · 2b9c58d — W&B decoupling + four correctness fixes (29 files, scrutinise this one)

These are behaviour changes, not refactors.

Training / W&B. 10 of 14 scripts only saved checkpoints when --track was
on, so training without W&B silently produced nothing on HuggingFace. VQGAN
additionally gated early stopping — real training logic — behind it, so under
default settings its transformer checkpoint was never written at all. Found by a
live run, not by tests.

iog was not an optimality gap. It stored the raw simulated objective while
cog/fog measured against the reference optimum. Verified with an oracle
generator that returns the reference design itself: it now scores iog ≈ 3e-07
where it previously reported the design's raw compliance.

Condition sampling couldn't serve half the problems. photonics2d declares
solver settings that aren't dataset columns; thermoelastic2d encodes boundary
conditions as 65×65 matrices. Scalar conditions now feed the model tensor while
the simulator still receives every condition — the physics is unchanged.

Multi-objective scores leaked. thermoelastic2d has a per-sample weight
spanning 0.0–1.0. Summing or averaging its three objectives meant a purely
structural sample was scored mostly on thermal compliance:

weight	sum	mean	weighted
1.0 (structural only)	42.31	14.10	0.30 ← correct
0.0 (thermal only)	42.31	14.10	42.00

Scalarization is now declared in the eval spec, and the evaluator refuses to
guess when it's absent.

Gaps ignored objective direction. photonics2d maximizes total_overlap,
so a design that beat the reference scored +0.3 and ranked last. Gaps are now
signed by ObjectiveDirection.

Also removes metrics.metrics() and simulate_failure_ratio() — both dead once
the evaluator landed. (This was the only remaining mypy error in the codebase.)

---

Verification

76 tests. Metric correctness is asserted by properties that must hold for any
correct implementation — mmd(X, X) == 0, collapsed generators score below
varied ones, a purely structural sample is unaffected by thermal compliance —
rather than by pinning historical numbers.

Verified live against HuggingFace with W&B disabled: trained two
hyperparameter configurations, confirmed they landed in separate packages,
loaded each back by fingerprint, evaluated, pushed the leaderboard incrementally,
and confirmed the first model's row was untouched. VQGAN was exercised
separately as the multi-file, multi-stage case. All test repos deleted after.

Ruff clean; mypy clean.

---

Migration notes for reviewers

• The lvae branch will need adapting. Its evaluate_*.py scripts call the
  removed metrics.metrics() and pass W&B artifact arguments that no longer
  exist. That was accepted deliberately rather than maintaining two evaluation
  paths.
• All checkpoints must be retrained, which was already the plan — the storage
  layout changed and there is no W&B read-fallback.
• surrogate_model/ is untouched and sits outside the contract; it's neither a
  generator nor evaluated.

Known limitations

• cog sums across steps and objectives while fog averages objectives — a
  pre-existing inconsistency, left alone rather than changed silently. Worth
  settling alongside the metric-suite work.
• dpp underflows toward zero on collapsed generators (a 1-epoch GAN scored
  8.9e-22), so it may lose ordering at the bottom of a real zoo. A log-domain
  form would fix it.
• Leaderboard submissions are internal for now. The Generator contract is
  the submission interface, so opening it later is a policy change, not a
  rewrite.

---

Update — review round 1 addressed (commits 3a2568e, 810529f)

All 13 of @SoheylM's comments are fixed, each with a threaded reply. Summary comment
below has the full breakdown; the two things worth knowing before re-reading the diff:

1. Evaluation now needs EngiBench from source, and CI pins a commit.
The two specs that would not resolve for Soheyl were not a case of datasets drifting
upstream — they were frozen against EngiBench main, whose photonics2d and
thermoelastic2d read the v1 datasets, while release 0.2.0 still points those two at
v0. On 0.2.0 they sample different data entirely. Specs now record the problem
definition they were frozen against, so that case reports itself instead of surfacing as
a hash mismatch. CI installs EngiBench at a pinned commit; the README says so.

2. Four changes are in the diff that nobody asked for. Flagged so they are not a
surprise: the EngiBench pin above; two extra bug fixes inside the feasibility work
(check_constraints raised on thermoelastic2d, and float64 designs were judged
infeasible against a float32 design space on dtype alone); a pytest job in CI, since the
repo ran only ruff and mypy and the requested spec test had nowhere to live; and the
deletion of two dead helpers this PR had itself added.

Also removed the sweep YAMLs and the per-algorithm notebook that should never have been
committed.

Testing: 136 tests, up from 111. CI green on all four checks.

Known limitation — image conditions

thermoelastic2d models are conditioned on 3 scalars while the simulator receives all 7
conditions, because its four 65x65 boundary masks cannot travel in a dense condition
tensor. Scoring is correct — simulate and optimize get the full conditions — but a
model cannot currently see where the part is held, loaded, or cooled unless it reads
ConditionBatch.dataset itself, as vqgan does for its own preprocessing. First-class
support is being prepared as a follow-up PR; it is purely additive to this contract
(new optional fields, no adapter changes), so it does not block this one.

---

Update — review round 2: making the board survive being public

All seven remaining comments are fixed, with a threaded reply on each. Two of
them turned out to be the tip of a larger problem, so this round also closes the
gap that would have stopped this leaderboard working as a public one.

The board was a self-report system

A row recorded a checkpoint hash but never a repo, and every default
pointed at IDEALLab. An outside contributor could neither publish weights nor
produce a row anyone could trace back to them, so every number was an assertion
nobody could falsify.

Rows now carry checkpoint_repo / checkpoint_path / checkpoint_revision /
checkpoint_hash — an address, not just a fingerprint — and
python -m engiopt.verify follows it: fetch the package at the recorded
revision, confirm it still hashes to what was scored, rebuild through the
registered adapter, score it again. Rows land verified=false and unranked
until a runner reproduces them.

Verification publishes its own numbers, not a pass/fail on the submitted
ones. Sampling from one seed on different hardware genuinely produces different
designs — CUDA and CPU draw different values from the same generator state — so
any tolerance loose enough for honest variation is loose enough for real
fudging. Re-scoring sidesteps that; the submitted value becomes a claim reported
as corroborated or not.

The runner is not privileged. Because the address is public, python -m engiopt.verify with no credentials and no --publish is an audit anyone can
run, which is what keeps the runner honest too.

The protocol is public, so three metrics are copyable by construction

The spec names the scored conditions, and the dataset supplies the optimal
design for each. mmd, iog and fog are therefore defined as closeness to
designs anyone can look up. Measured on the real beams2d spec, against the
trained models in this repo:

	mmd	viol	copy_rate	cond_sens
lookup table	0.000	0.00	1.00	0.0	flagged memorized
cgan_cnn_2d	0.561	0.92	0.00	0.171
gan_cnn_2d	1.049	1.00	0.00	0.0	unconditional, correctly unflagged
vqgan	0.007	0.46	0.02	0.386

A lookup table beats every trained model on both headline metrics. That is not
an implementation flaw and it cannot be fixed by hiding which rows are scored,
since the whole dataset is public. So the board measures retrieval instead:
novelty / copy_rate catch it, and cond_sens catches a model that ignores
conditions it declares it uses.

Both are diagnostic, with no ranking direction, and that is the load-bearing
choice. Ranking on novelty would seat pure noise in first place — zero means
retrieval, but large means only "unlike the data", which a broken model also
achieves. Closing one gaming vector by opening another is not progress. Flagged
rows are published and left out of the ordering: deleting a submission is
moderation, declining to rank one is a statement about what its number measures,
and only the second scales.

cond_sens is not a new feature so much as a claim this PR had already made —
registry.py declared a conditions metric family and core.py referred three
times to "conditional-adherence metrics", with no such metric existing. The
alternative was deleting the claims.

Other ways a score could drift from the thing scored

• required_seeds. An entry must cover seeds 1, 2, 3 to be ranked. A count
  would not do: it still permits running twenty and publishing the best three,
  which turns a median into a maximum while every individual row stays honest.
• ROW_KEY gains checkpoint_repo. Two contributors who both train
  cgan_cnn_2d on default hyperparameters and seed 1 produced the same key and
  different weights, so either one's push would silently replace the other's
  verified row.
• A silent skip. --generators gan_cnn_2d vqgan --config-fingerprints gan_cnn_2d:6293adb3 scored gan_cnn_2d and never mentioned vqgan — the empty
  fingerprint list meant the loop body never ran, producing no row, no load
  attempt and no error. Found by running it, not by a test.

Verification

255 tests, up from 201; CI green on all four checks with no skips.

Exercised against the live Hub rather than fakes: a lookup table scored on the
real beams2d spec (copy_rate=1.00); four published checkpoints loaded and
scored through the contract, including VQGAN's multi-file case; the documented
diffusion_2d_cond --seeds 1 2 3 run produced three rows collapsing into one
entry with n_seeds=3; all four committed specs still reproduce their frozen
digests; and 143 published packages were checked to confirm the new
incomplete-package refusal rejects none of them.

New docs: LEADERBOARD.md — the trust model, the flags, and
the design of the v2 spec that would actually close the copying hole rather
than price it (scoring on off-dataset conditions, which needs an optimizer run
per condition and so is not built here).

Still open, deliberately

Migration scope. Ten of the fourteen registered generators have no published
checkpoint. --list-generators --check-availability now reports that per
generator instead of implying all fourteen are usable — but reporting is not
choosing between migrating them, keeping a fallback, and narrowing the registry.
That is a project decision, so #73 should stay open.

🤖 Generated with Claude Code

[SoheylM]

The generator interface and shared evaluator make sense to me, and removing the duplicated evaluation scripts is a good change. I am requesting changes because several parts of the documented evaluation flow do not work end to end yet. I have left the concrete code issues inline.

There are four cross-cutting points as well:

• Checkpoint migration: Issue #73 says to remove the migration playbook after the W&B migration is complete, but the issue is still open and this PR removes both the playbook and W&B loading. On 2026-08-03, IDEALLab/engiopt-cgan-cnn-2d contained only .gitattributes and README.md; IDEALLab/engiopt-vqgan and IDEALLab/engiopt-gan-cnn-2d did not exist. The README's cGAN evaluation command cannot currently load beams2d/seed_1. Please publish and verify the required HF packages before removing the old path, or keep the migration support until that is done.
• Documentation: The README still describes separate training and evaluation scripts, says the power-electronics optimizer accepts legacy W&B artifacts, and links to the deleted migration playbook. run_pe_optimization.py still uses W&B references in its example and argument descriptions, while Generator.resolve_checkpoint and the evaluation CLI help still list wandb as a model source. These should match the final migration decision.
• Lint CI: With Ruff 0.15.11, ruff check . reports 20 I001 errors on this commit. The GitHub job is green because it runs ruff check --fix in the temporary checkout without checking whether files changed. Please commit the import fixes and run plain ruff check . in CI. Pinning Ruff would also make the check stable.
• Workshop integration: The open DCC workshop branch still imports engiopt.cgan_cnn_2d... in notebook_helpers.py, which is used by the participant notebooks. Please update that branch before these changes reach participants, or keep a temporary import alias if the old path must remain supported.

Verification on the PR head: 76 tests passed with 3 warnings; two of four committed specs failed to load; the thermoelastic adapter shape error and mixed-ranking result were reproduced; the documented cGAN evaluation loaded no model and exited with status 0; git diff --check passed.

[SoheylM]

I ran Evaluator.for_problem for all four committed specs using the current EngiBench 0.2.0 package. beams2d/v1 and heatconduction2d/v1 load, but photonics2d/v1 fails with digest ade3d18084b31b5e expected and 1f1bbaa4003ded80 found. thermoelastic2d/v1 fails with 3a1d2ae6c45add7f expected and cb1c6e77cbec5883 found. Both JSON files also record EngiBench 0.2.0, so the package version does not identify the exact dataset used to create them. The digest detects the change but cannot recover the original evaluation rows. Could we pin and load a dataset revision in the spec, or store the selected conditions and reference designs with it? I would also add a CI test that resolves every committed spec.

[mkeeler43]

Fixed — and chasing your repro turned up the actual cause, which is not what either of
us assumed.

It was never upstream dataset drift. The two specs that failed for you failed because
they were frozen against EngiBench main, not the 0.2.0 release you had installed. Those
two problems point at different datasets in the two versions:

problem	EngiBench 0.2.0 (PyPI)	EngiBench main	your result
beams2d	beams_2d_50_100_v0	same	resolved
heatconduction2d	heat_conduction_2d_v0	same	resolved
photonics2d	photonics_2d_120_120_**v0**	..._**v1**	digest mismatch
thermoelastic2d	thermoelastic_2d_**v0**	..._**v1**	digest mismatch

So you were sampling a different dataset entirely, and the digest correctly refused. Your
underlying point stands exactly as written — the recorded EngiBench version did not
identify the data — it was just more true than it looked: the problem definition is
what selects the dataset, and a version string pins neither.

Three changes, smallest first:

1. Specs pin the dataset. dataset_id + dataset_revision, recorded at freeze time,
   and resolve loads that exact revision. A later upload to the dataset repo can no
   longer change what a published row means.
2. Specs record the problem definition they were frozen against (problem_conditions,
   plus engibench_version now carrying the git sha). resolve checks this before the
   digest, so the case you hit reports itself:

   Eval spec thermoelastic2d/v1 was frozen against a different definition of 'thermoelastic2d'.
     conditions expected: ['fixed_elements', ..., 'volume_fraction_target', 'rmin', 'weight']
     conditions found:    ['fixed_elements', ..., 'volfrac', 'rmin', 'weight']
     dataset expected:    IDEALLab/thermoelastic_2d_v1
     dataset found:       IDEALLab/thermoelastic_2d_v0
   

   rather than two hashes that will never match.
3. CI installs EngiBench from git at a pinned commit, since no PyPI release has the v1
   problems yet, and the README now says evaluation needs EngiBench from source. This is
   the part worth your attention — it is a real constraint on anyone running the specs,
   and it should probably relax to a version pin once EngiBench releases.

All four specs are re-frozen and now resolve; the two that failed for you resolve. Nothing
had been published to a leaderboard, so re-freezing cost no history.

CI test added as you asked, plus offline checks so it fails fast without the network:

• test_committed_spec_reproduces_its_frozen_conditions — resolves every committed spec
  (@pytest.mark.network, parametrised over engiopt/specs/*/*.json, so a new spec is
  covered the moment it is added)
• test_committed_spec_pins_its_dataset — a spec without a pinned revision fails review
• test_a_differently_defined_problem_reports_itself — the message above
• test_committed_spec_loads_and_round_trips, ..._requests_registered_metrics

The repo had no pytest job at all — only ruff and mypy — so I added one.

[SoheylM]

The field documentation says this digest covers the sampled conditions and reference designs, but _digest only receives the selected indices and reference designs. A condition value can therefore change without changing the digest. Please include the condition names and values in the hash, using a stable representation, in both freeze and resolve.

[mkeeler43]

Fixed. _digest now takes the sampled conditions dataset and hashes, in sorted column
order, each condition's name and its values, alongside the indices and reference designs.
Numeric columns are rounded to 8 decimals before hashing so float noise well below any
tolerance the metrics use can't invalidate a spec; array-valued columns
(thermoelastic2d's boundary matrices) are covered the same way, and non-numeric columns
fall back to a sorted JSON form. freeze and resolve share the one function, so they
can't drift.

Four tests pin the behaviour you were protecting: changing a condition value, renaming a
column, and editing an array-valued condition each change the digest; reordering columns
does not.

[SoheylM]

n_conds is described as the number of scalar conditions, but this returns all condition keys. For the current thermoelastic2d problem, the evaluator produces 3 scalar columns (volfrac, rmin, and weight) while the problem has 7 condition keys because four boundary conditions are matrices. cgan_cnn_2d builds for 7 inputs and then fails with shape '[2, 7, 1, 1]' is invalid for input of size 6. Other conditional adapters and training scripts use the same all-keys assumption, and VQGAN tries to stack the scalar and matrix columns. Please use the actual scalar condition keys, or save the expected condition schema with the checkpoint, throughout training, loading, and sampling. An adapter test using the thermoelastic condition schema would cover this.

[mkeeler43]

This was the real bug in the PR — thank you, the thermoelastic2d repro was exactly right.

Both halves of your suggestion are now in, because they solve different failures:

One definition of the scalar schema. New engiopt.transforms.condition_keys(problem)
is the single answer to "which conditions does a model consume", and training scripts,
dataloaders, adapters, and sampling all use it. Generator.n_conds is now
len(self.condition_keys), not len(problem.conditions_keys). Nine training scripts and
seven adapters updated; vqgan's stacking of scalar and matrix columns is fixed by the
same change.

The schema travels with the checkpoint. save_checkpoint_package(..., condition_keys=...)
records it in metadata.json, and condition_keys_for(problem, resolved) prefers the
recorded schema over the problem's current one — so a checkpoint keeps loading after a
problem gains a condition. Checkpoints written before this fall back to the problem's
current scalar keys, which is what those runs actually used.

I also made the mismatch loud rather than latent: if the conditions handed to sample
don't match the columns the checkpoint was trained on, it raises naming both, instead of
failing with a reshape error deep inside the network — or worse, silently conditioning on
the wrong numbers.

Your repro, before and after:

before: RuntimeError: shape '[2, 7, 1, 1]' is invalid for input of size 6
after:  problem.conditions_keys: 7 -> scalar: ('volume_fraction_target', 'rmin', 'weight')
        condition tensor: (50, 3)   sampled: (50, 64, 64)   n_conds: 3

Tests in tests/test_condition_schema.py use the thermoelastic schema as you suggested:
7 declared conditions, 3 columns, sampling succeeds, mismatched schemas rejected, recorded
schema preferred over the problem's.

[SoheylM]

ROW_KEY includes config_fingerprint and spec_version, but rank groups only by problem_id and algo_id. This combines different hyperparameters and evaluation protocols. For example, a cfg_a/v1 score of 0.1 and a cfg_b/v2 score of 0.9 are reported as one score of 0.5 with n_seeds=2, even when both rows use seed 1. Please require one config/spec before ranking, or include config_fingerprint and spec_version in the grouping. disagreement should follow the same rule.

[mkeeler43]

Fixed by including both columns, which is the option that keeps a sweep rankable.

New ENTRY_KEY = [problem_id, algo_id, config_fingerprint, spec_version] — ROW_KEY
minus seed, since seeds are what a ranking aggregates over and nothing else should be.
rank groups by it, and disagreement uses the same helper, so the two can't diverge.
Your example is now two rows with n_seeds=1 each rather than one averaged 0.5.

entry_key(frame) raises if none of those columns are present, rather than silently
pooling the whole table into one number.

[SoheylM]

This catches every exception raised by hf_hub_download, including authentication, network, and server errors, and returns an empty frame. push_to_hub can then upload only the new rows and remove the rows already on the Hub. Please catch only the specific repository/file-not-found cases and let other download errors stop the update. A test for a transient download failure would be useful here.

[mkeeler43]

Fixed. load_from_hub now catches only EntryNotFoundError, RepositoryNotFoundError,
and RevisionNotFoundError — the genuine "nothing published yet" cases. Auth, network,
and server errors propagate, so a transient failure can no longer be laundered into an
empty board that the next push turns into a deletion.

Tests in tests/test_leaderboard_publishing.py cover both directions: a 503 and a 401 each
raise; a missing repo and a missing file each read as empty.

[SoheylM]

With the documented cgan_cnn_2d, beams2d, seed 1 command, checkpoint loading fails, this message is printed, and the process exits with status 0. That makes an empty batch run look successful. Please return a non-zero status when none of the explicitly requested models can be loaded or evaluated, while keeping the current skip behavior when only part of a multi-model run fails.

[mkeeler43]

Fixed. main returns a status and __main__ exits with it:

• nothing could be loaded → 1
• every generator failed to evaluate → 1
• some loaded, some failed → 0 (your "keep the current skip behaviour" case)
• everything skipped by --skip-existing → 0, since that's a finished job, not a failed one

The documented command you ran now prints the same message and exits 1.

[SoheylM]

The timer stops before raw.detach().cpu().numpy(). CUDA and MPS operations can still be pending at this point, so sample_seconds can under-report the actual sampling time. Please synchronize the active device before stopping the timer, or stop after the CPU transfer and state that the transfer is included.

[mkeeler43]

Fixed by synchronising, which keeps sample_seconds measuring sampling rather than the
CPU transfer. Generator._synchronize_device calls th.cuda.synchronize(self.device) or
th.mps.synchronize() as appropriate and is a no-op on CPU; sample calls it before
stopping the timer. Overridable, and there's a test asserting sample waits.

[SoheylM]

Every committed spec requests viol, but this only calculates it when a condition named volfrac or volume exists. photonics2d has neither, so after its digest issue is fixed this metric will return NaN. The a or b lookup also ignores a valid target of 0.0. Please define feasibility for each spec, use problem.check_constraints where appropriate, or remove viol from specs where it has no meaning.

[mkeeler43]

Fixed, taking your check_constraints suggestion as the base definition so the metric is
defined for every problem rather than removed from some specs:

1. problem.check_constraints always applies — EngiBench's own contract, which a new
   problem gets for free by declaring constraints
2. plus the volume budget named by the spec's new volume_condition, for problems that
   have one

Specs now declare it explicitly instead of the code guessing: volfrac (beams2d),
volume (heatconduction2d), volume_fraction_target (thermoelastic2d), none (photonics2d).
Worth noting the old auto-detect never matched thermoelastic2d either — it looked for
volfrac/volume and the key is volume_fraction_target — so viol was silently absent
there too. The a or b lookup is gone; an explicit is not None test means a target of
0.0 counts.

Implementing this surfaced two further problems worth flagging, both fixed and tested:

• check_constraints raised on thermoelastic2d. The HF dataset returns its boundary
  matrices as nested lists and EngiBench's bound check does lower <= value, giving
  TypeError: '<=' not supported between 'float' and 'list'. We coerce list-valued
  conditions to arrays before the call. This may be worth a fix upstream in EngiBench too.
• Dtype alone made designs infeasible. thermoelastic2d's design space is float32 while
  its designs are float64, and Box.contains rejects an uncastable dtype outright — so
  every design, including the dataset's own optima, scored as violating. We cast to the
  space's dtype first, which makes the check about the values.

Verified on all four problems: reference designs feasible, out-of-bounds designs not.

[SoheylM]

For a non-default config, the config-specific package is uploaded while this still points to {problem}/seed_n. That call does not upload the canonical path, and only the returned info value is cleared after the config metadata has already been written. The resulting metadata.json can therefore point to a canonical package that this run did not create. Please build metadata separately for the config-specific and canonical uploads.

[mkeeler43]

Fixed as you described — metadata is now built per upload. A shared base_metadata carries
the run-level facts, and each _upload_package_to_hf call gets {**base_metadata, "hf_package_path": <the path being written>}. The returned info records only paths this
run actually claimed.

Two tests: a non-default run writes one package whose metadata names its own config path
(and info["hf_package_path"] is None), and a default run writes two packages that each
describe themselves.

[SoheylM]

The beams, heat-conduction, and photonics sweep files still run engiopt/cgan_cnn_2d/cgan_cnn_2d.py, which no longer exists after this move. Please change all three to engiopt/generators/cgan_cnn_2d/cgan_cnn_2d.py and use the current --save-model option.

[mkeeler43]

Resolved by removing the files. These three were never meant to be committed — they were
local sweep configs that got swept into the file-move commit. Removed here, along with the
two pre-existing sweep YAMLs and cgan_vae/sample_script.ipynb that this PR's reorg moved.

The two affected READMEs referenced the deleted evaluate_*.py scripts as well, so they now
point at python -m engiopt.evaluate.

[mkeeler43]

Thanks for this — the review caught one genuine correctness bug (the condition count) and
several things that would have quietly corrupted the leaderboard once more than one person
was writing to it. All 13 are addressed, and each has a threaded reply above.

Fixes for your comments

Correctness

• Condition schema unified and recorded per checkpoint; the thermoelastic2d failure is fixed
  and mismatches now raise instead of reshaping wrongly (Add CGAN Model #3)
• Spec digest covers condition names and values, including array-valued ones ([Proposal] Metrics computation #2)
• Specs pin a dataset revision and the problem definition; all four re-frozen and
  resolving, including the two that failed for you (Add GAN 1D implementation #1)
• viol defined for every problem via check_constraints plus a spec-declared volume
  budget (made algos and metrics compatible with eval_model.py files #11)

Leaderboard integrity

• Ranking keeps configurations and spec versions apart (Diffusion 1d #4)
• Download failures no longer read as an empty board (Bad Import engibench.utils.all_problems #5)
• Publishing is conditional on the revision it read, with re-merge on conflict (Fix import #6)
• Rows carry checkpoint revision, weight hash, and code version (Fix problem build #7)

Usability

• --local-model-dir makes the local source actually reachable (Tag artifacts with seed #8)
• Non-zero exit when nothing evaluates (pyproject.toml Specifies engilearn Instead of engiopt #9)
• Device-synced sample timing (Remove occurences of engilearn #10)
• Per-package checkpoint metadata (Feature/surrogate optimization #12)
• Sweep configs removed rather than repaired — they should never have been committed (Add vscode to gitignore #13)

Changes beyond your comments, and why

Four things are in this branch that you did not ask for. Flagging them so nothing in the
diff is a surprise:

1. EngiBench must now come from git, and CI pins a commit. Not a preference — no PyPI
   release has the photonics2d/thermoelastic2d v1 problems, so on 0.2.0 those two specs
   cannot resolve at all. This is the root cause of what you reported in Add GAN 1D implementation #1. See that reply;
   it is the change most worth a second opinion.
2. Two extra bug fixes inside made algos and metrics compatible with eval_model.py files #11. Implementing feasibility surfaced that
   check_constraints raised on thermoelastic2d (HuggingFace returns its boundary
   matrices as lists, which EngiBench's bound checks cannot compare), and that float64
   designs were judged infeasible against a float32 design space on dtype alone — which
   marked that problem's own dataset optima as violations. Both are fixed and tested here
   because viol is wrong without them. The list/array crash may deserve a fix in EngiBench
   too.
3. A pytest job in CI. The repo ran only ruff and mypy, so the spec test you asked for
   in Add GAN 1D implementation #1 had nowhere to live. 136 tests now run on every push (111 before).
4. Two dead helpers deleted — _discover_reference_files (a near-duplicate of live code,
   no callers) and problem_id_of (public, undocumented, unused). Both were added by this
   PR, so this is removing our own dead weight rather than unrelated tidying.

Known limitation, not addressed here

thermoelastic2d models are conditioned on 3 scalars while the simulator receives all 7
conditions, because the four 65x65 boundary matrices cannot travel in a dense condition
tensor. Scoring is correct — simulate and optimize get the full conditions — but a
model cannot currently see where the part is bolted or heated unless it reads
ConditionBatch.dataset itself, as vqgan does for its own preprocessing. Giving image
conditions first-class support in the contract is worth a follow-up; it is a modelling
capability rather than a bug, and it predates this PR.

[SoheylM]

Thank you for the detailed follow-up. Several original issues are fixed, including local loading, exit status, timing, checkpoint metadata, and grouping configurations into separate entries. The remaining inline findings still affect reproducibility or leaderboard correctness, so I am keeping Changes Requested.

The migration and repository cleanup also remain unfinished. Issue #73 is open, the public HF repositories still do not provide the documented checkpoint packages, and the README links to the deleted migration playbook while retaining obsolete W&B source instructions. ruff check . reports 20 import-order errors; CI stays green because it runs with --fix. The DCC workshop branch also still imports the old pre-move module path.

Verification on 810529f: the local suite reports 134 passed and 2 skipped with PyPI EngiBench; all four spec-resolution tests pass when the actual pinned EngiBench source is imported; git diff --check passes; the worktree is clean.

[SoheylM]

CI is green because this skips the two cases the new job is supposed to verify. In run 31012275781, photonics2d/v1 and thermoelastic2d/v1 were skipped, leaving 134 passed and 2 skipped. The git install does not replace the PyPI copy installed earlier because both report version 0.2.0. I ran these four tests with EngiBench source at 0a028c03..., and all four pass. Please force-install that revision and let ProblemDefinitionMismatchError fail this CI test; otherwise the pin can break while CI remains green.

[mkeeler43]

You are right, and this is the one that mattered. I reported CI as green; it was
green on a false basis. Thank you for checking the run rather than the tick.

Diagnosis matches yours exactly: the git build reports version 0.2.0, the same as the
PyPI build, so pip install "engibench @ git+..." saw the requirement already satisfied
and did nothing. The specs were then verified against the EngiBench that cannot resolve
them, and the skip hid it.

Two changes:

- run: pip install --force-reinstall --no-deps "engibench @ git+...@0a028c03d02b..."
- run: python -c "import engibench.problems.thermoelastic2d.v0 as m; assert m.ThermoElastic2D.dataset_id.endswith('_v1'), m.ThermoElastic2D.dataset_id"

--force-reinstall is the fix; the assertion is there so that if this ever silently
reverts, the job fails at the install step rather than in a way that looks like a passing
test run.

And the skip is gone — ProblemDefinitionMismatchError now fails the test, as you asked.
Your reasoning is the deciding one: a mismatch means the pin stopped taking effect, which
is precisely the condition this job exists to catch, so it must never be the quiet
outcome. That also deleted the try/except rather than adding to it.

[SoheylM]

A recorded checkpoint schema cannot currently be a subset of the evaluator scalar keys. VQGAN defaults to dropping constant conditions and records the reduced schema, but the evaluator supplies all scalar keys, so this equality check raises before VQGAN can preprocess them. The same happens after a problem gains a scalar condition, despite the compatibility claim above. If the checkpoint schema is authoritative, please project and reorder a supplied superset to those keys, rejecting only missing required keys.

[mkeeler43]

Right, and this was a bug I introduced last round: the strict equality check rejected
VQGAN before it could run at all.

Implemented as you described. _select_trained_columns projects a supplied superset onto
the checkpoint's recorded keys, in the checkpoint's order, and raises only when a key the
model needs is genuinely absent:

volfrac, rmin, forcedist  (evaluator)  ->  volfrac  (checkpoint recorded)   projected
volfrac, rmin             (evaluator)  ->  volfrac, brand_new               raises
volfrac, rmin             (evaluator)  ->  rmin, volfrac                    reordered

This is better than my original in a way worth naming: the checkpoint's schema is now
authoritative in fact, not just in the docstring. It also covers your "after a problem
gains a scalar condition" case, which my version claimed to support and did not.

Tests in tests/test_condition_schema.py cover projection, reordering, and the
missing-key error.

[SoheylM]

This still begins with every condition column. For thermoelastic data that mixes scalars with 65x65 matrices, so drop_constant or the final stack raises the same mixed-shape error. It also re-fits constant removal and normalization on the evaluation sample, whereas training fitted them on the training split. Please save the selected keys and normalization statistics with the checkpoint and apply exactly those here. A VQGAN-specific mixed-condition test would cover this.

[mkeeler43]

Agreed on all three points, and this was the most serious of the round: the discarded
_mean, _std meant the network received conditions on a different scale than it was
trained on, silently.

Rather than fix the adapter, the contract now does the job for every model. The principle:
a model that preprocesses its conditions records how, and the contract replays it.

• Training records what it fitted: save_checkpoint_package(..., condition_keys=..., condition_stats={"mean": [...], "std": [...]}). VQGAN passes the statistics normalize
  returned instead of throwing them away.
• Generator.sample projects to the recorded keys (see C) and applies the recorded
  scaling, so _sample receives the conditions in exactly the form training used.
• VQGANGenerator._condition_tensor is deleted. The adapter now uses
  conditions.require_tensor(...) like every other model.

That removes the mixed-shape crash too: the projection works on the scalar condition
tensor and never touches the 65x65 matrices, so thermoelastic no longer reaches
drop_constant.

The general mechanism was deliberate — we expect more two-stage models, and they should
get this pathway without writing code for it.

Tests: recorded statistics are replayed rather than refitted (with the evaluation sample's
own mean chosen to differ, so a regression fails), and a model with no recorded statistics
is left untouched.

[SoheylM]

Conditional VQGAN loading requires and uses cvqgan.pth, but it is absent from checkpoint_files. The resolver content hash therefore covers only vqgan.pth and transformer.pth: two packages with different condition encoders receive the same checkpoint_hash even though they generate differently. Please include every weight file used by build in the provenance hash and test that changing cvqgan.pth changes it.

[mkeeler43]

Right. Fixed more generally than by extending checkpoint_files, because cvqgan.pth
cannot go in that list — it is required for conditional runs and absent for unconditional
ones, so making it mandatory would break the unconditional variant.

hash_package_contents(root_dir) now hashes every file in the package except
run_config.json and metadata.json, rather than the declared list. Any weight file a
model loads is covered whether or not it declared it, which is the property you actually
want and which generalizes past VQGAN.

metadata.json is excluded deliberately: it records the package path and revision, which
differ between the two locations a single run writes, so including it would give identical
weights two different hashes.

Two tests: changing cvqgan.pth changes the hash; changing metadata.json does not.

[SoheylM]

These exceptions need separate handling. Hugging Face can raise RepositoryNotFoundError for a private repository without access, including 401/403 responses, so authentication can still become an empty board. When only leaderboard.csv is missing, repo_info has already returned a revision, but this discards it; I reproduced the first upload using parent_commit=None, leaving two first publishers able to overwrite one another. Please treat only a genuine 404 repository as (empty, None), return (empty, revision) for a missing file, and propagate authorization and revision errors.

[mkeeler43]

Both correct, and the second is the more dangerous one — thank you for reproducing it.

Split into two steps, as you described:

• repo_info failing with RepositoryNotFoundError is only an empty board when it is a
  genuine 404. A 401/403 re-raises, since a repo you cannot see is not a repo with no rows.
• A missing leaderboard.csv in an existing repo now returns (empty, revision). The
  revision becomes the parent_commit of the first publish, so two jobs racing to publish
  first can no longer overwrite one another.

Tests cover both: a private repo raises, and the first publish into an existing empty repo
carries its revision as parent_commit.

[SoheylM]

ENTRY_KEY prevents different configurations and specs from being averaged together, but ranks are still assigned globally across the entire frame. Different problems and spec versions therefore compete with each other; I reproduced one problem receiving ranks 1/2 and another receiving 3/4. Please rank independently within (problem_id, spec_version), with each partition starting at 1. disagreement should use the same partitioning.

[mkeeler43]

Right — I fixed the grouping and left the ranking global, which was half a fix.

RANK_PARTITION = ["problem_id", "spec_version"], and ranks restart at 1 within each
partition via groupby(partition).cumcount() + 1. disagreement calls rank, so it
follows automatically.

Two tests: two problems each get ranks 1/2 rather than 1/2/3/4, and two spec versions of
one problem each get rank 1.

[SoheylM]

--skip-existing does not compare checkpoint_hash. If the same configuration and seed are retrained and the canonical package now contains different weights, this skips evaluation because the old row matches these five fields. Please include the loaded checkpoint hash when the board supports it, and add a test showing that a new hash is not skipped.

[mkeeler43]

Agreed. already_evaluated now takes checkpoint_hash, and --skip-existing passes the
loaded generator's.

One deliberate wrinkle: a row written before the hash column existed has nothing to
compare, so it is treated as a match and still skipped. Forcing a re-run of every
historical row on the first run after this lands would be a surprising cost for no
information.

Three tests: a new hash is not skipped, the same hash is, and a hashless row is.

[SoheylM]

viol cannot reliably report an infeasible design because feasibility is checked only after optimization and simulation. If the optimizer rejects an invalid starting point, evaluation raises before is_infeasible runs; I reproduced that behavior. Requesting only viol also performs the full optimizer pass. Please compute feasibility independently before optimization and keep only the gap metrics in the shared optimization result.

[mkeeler43]

Good catch, and fixing it made things smaller rather than larger.

Feasibility left OptimizationResults entirely and is now EvaluationContext.feasibility,
computed by a constraint check over the generated designs before any solver runs. Both
consequences you named are gone: an optimizer that refuses an invalid starting point no
longer takes the metric down with it, and requesting viol alone no longer runs a full
optimization pass.

viol is therefore now registered as cheap, which also means it appears in the default
non---include-expensive run — a better default, since feasibility is exactly what you
want to see before spending simulator time.

Two tests: feasibility still reports when optimize raises, and computing viol leaves
optimize_calls == 0.

[mkeeler43]

All nine addressed; eight adopted, one half-declined with reasoning (B).

The one that mattered most is A: you were right that CI was green because it skipped
the two specs it exists to verify. My EngiBench pin was a no-op — the git build reports the
same version string as the PyPI build, so pip kept the wrong copy. I had reported this PR
as "CI green" on that basis, which was wrong. It is now a --force-reinstall with an
explicit assertion that the pin took effect, and a spec mismatch fails rather than skips.

Four of these were defects the previous round introduced (C, F, G, and A itself). Two of
the fixes made the code smaller rather than larger:

• viol is now cheap — decoupled from the optimizer pass, so it survives an optimizer
  that rejects an invalid design, and costs a constraint check rather than a full run.
• VQGAN's bespoke preprocessing is deleted, not patched. The contract now replays
  whatever preprocessing a checkpoint recorded, so any two-stage model gets the pathway
  without writing code for it. This also fixed a silent correctness bug: VQGAN was
  refitting its condition normalization on the 50 evaluation rows and discarding the
  training statistics, so the network saw a scale it was never trained on.

Where I would push back — B, second half only. Recording the EngiBench revision is
right and is done. Enforcing it at resolution would refuse to evaluate for any contributor
whose EngiBench differs by a commit, and putting it in ranking identity would partition the
board per build, so two models could not be compared unless scored on byte-identical
EngiBench. Traceability is what the concern justifies, and the revision is recorded in the
spec with code_version already on every row. Happy to add engibench_version to the
provenance columns as well if you want it visible per row — that is the useful half
without the cost. Say the word and I will.

153 tests, up from 136.

[SoheylM]

I rechecked the latest head. The condition handling, VQGAN preprocessing, feasibility, ranking, Hub publishing, and CI spec verification now look correct. The pinned suite passes all 155 tests without skips.

One documentation cleanup from the previous review summary remains. This PR removes the per-model evaluation scripts, removes W&B as a checkpoint source, and deletes docs/checkpoint_migration_playbook.md, but the README still says there are two scripts per algorithm, still lists legacy W&B checkpoint references, and still links to the deleted playbook. run_pe_optimization.py and one Generator.resolve_checkpoint docstring retain the removed W&B source as well. Please update those references. W&B experiment tracking and dashboard documentation can remain; only the checkpoint-loading claims are obsolete.

The documented default HF repository also still has no checkpoint package. Before presenting the evaluation command as runnable, please either publish that package, retain the migration path until issue #73 is complete, or state clearly that pretrained checkpoints are not available yet.

The remaining inline comments are limited to checkpoint identity, replaying training-fitted normalizers, hashless --skip-existing behavior, and runtime EngiBench provenance. I am not requesting further VQGAN work, a visual leaderboard, repository-wide Ruff cleanup, or workshop-branch changes in this PR.

[mkeeler43]

Thanks — all four of the latest round are addressed, each in its own thread. Summary here,
plus the cross-cutting items, plus three findings from verifying against the finished
checkpoint pool that you should know before re-reviewing.

The four inline items

	Item	Outcome
1	metrics.json inside the checkpoint hash	Fixed. Also found a second instance in _discover_package_files
2	Five adapters refitting normalizers at load	Fixed for all five; bounds recorded and replayed
3	Hashless rows treated as a match	Took your version; legacy-row test updated
4	Runtime engibench_version, and the git -C bug	Both done; the sha logic now uses PEP 610 direct_url.json first

Cross-cutting

Checkpoint migration / issue #73. The precondition you set is now met, and I checked
rather than assuming. All four architectures load from HF and score end to end:

beams2d  vqgan        mmd=0.0103  viol=0.36    (98 of 102 runs complete)
beams2d  cgan_cnn_2d  mmd=0.0277  viol=0.38

IDEALLab/engiopt-cgan-cnn-2d and -gan-cnn-2d hold 138 packages each,
-diffusion-2d-cond 139, -vqgan 102, -constrained-plvae-2d 54. So the W&B path is no
longer load-bearing for anything, and I think #73 can close with this PR rather than block
it. If you would rather keep it open until the leaderboard is actually published, that is
reasonable too — but the checkpoints themselves are there and verified.

Lint CI. Confirmed your diagnosis exactly: ruff check . reported 20 I001 on the
old head, and the job was green only because it ran --fix inside the runner. --fix is
gone and the 20 violations are committed. ruff check . is now clean at the un-fixed
setting CI uses.

Documentation. README no longer advertises W&B artifact refs or links the deleted
migration playbook; run_pe_optimization.py's argument docs and usage example are updated;
resolve_checkpoint no longer lists wandb as a source. The README also gained a
leaderboard section, which was previously undocumented — --push-to, --skip-existing,
--attach-metrics, and how an outside contributor submits a model.

DCC workshop branch. Here I would push back on the remedy rather than the observation.
The import break is real, but that branch is being overhauled separately and its notebooks
are changing substantially for IDETC, so a temporary import alias would be dead weight
added to main to serve a branch that will not use it by the time participants see it.
I would rather fix the import as part of that overhaul, and I am happy for that to be a
stated dependency of the workshop rather than of this PR.

Three findings from verifying against the pool

1. Nothing in a sweep writes the canonical path — the documented command is broken.
{problem}/seed_{seed} is what --seeds 1 resolves, and only a default-hyperparameter run
writes it. Every sweep arm sets at least one flag, so cgan_cnn_2d and gan_cnn_2d have
138 packages each and zero canonical packages. The README command fails for both.
(vqgan and diffusion_2d_cond do have them, but only because their center configuration
coincidentally equals the script defaults — luck, not design.)

Two changes: the resolution error now lists what the repo actually holds instead of a bare
"not found", and python -m engiopt.promote_checkpoint promotes an existing arm to the
canonical path rather than spending a few hundred GPU-hours retraining a model we have.

2. A correction to something I said earlier. On 2026-08-06 I reported that VQGAN's
packages held nothing but cvqgan.pth and attributed it to walltime exhaustion. That was
wrong: I sampled the Hub mid-sweep, and VQGAN uploads after each of its three stages, so a
run still training is indistinguishable from one that died. The pool finished at the
original epoch settings, 98 of 102 complete. I had staged an epoch-budget change on the
back of that diagnosis and have dropped it.

The 4 that genuinely did not finish are still the reason the completeness marker is worth
having: they hold vqgan.pth and no transformer.pth, and without package_complete they
look exactly like finished packages until you try to load one.

3. mypy in CI is passing vacuously. Not fixed here, flagging it because it is the
same shape as the two you already caught. The job installs only mypy numpy pytest, so
with torch/gymnasium/diffusers absent the ignore_missing_imports = true overrides turn
every library type into Any. Reproduced both ways:

• CI's exact environment: Success: no issues found in 74 source files
• Same commit with the real dependency stack: 61 errors in 10 files

All 61 predate this PR (63 on the previous head, so this branch reduces it by 2). Fixing it
means either installing the stack in that job or committing to burning down 61 pre-existing
errors, so I have deliberately left it rather than expand this PR further. Worth its own
issue.

Also worth flagging, deliberately not changed

dpp is closer to its floor than it looks. The determinant is a product of n eigenvalues
below 1, so it decays exponentially: on the two real checkpoints above it returned 1.3e-29
and 8.4e-15, and on Gaussian designs at sigma=10 it hits exactly 0.0 from around
n=200, at which point every model ties while the column still looks like a score. Two
training scripts already work around this — cgan_cnn_3d and cgan_vae both log
np.log(max(final_dpp, tiny)).

I had a log_dpp metric written and dropped it from this PR: at the n=50 the specs draw,
dpp is nonzero and ordinally valid, so nothing here is actually broken by it, and adding a
metric no spec references would have shipped dead code into an already large PR. Suggest a
spec v2 adopting it as a separate change — happy to open that if you agree.

Verification

CI on e6231bc: 201 passed, 0 skipped — counts rather than the badge, since the skip
was the failure mode last round; the four spec-resolution tests run for real. Locally:
ruff check . and ruff format --check clean without --fix, pre-commit including pyright
green, worktree clean. Every claim above about the Hub was checked against the live repos
rather than inferred.

[SoheylM]

Thanks for the substantial follow-up. The earlier concerns around fitted preprocessing, null checkpoint hashes, EngiBench provenance, Ruff CI, and obsolete W&B documentation have been addressed. I reran the complete test and quality suite against the pinned EngiBench revision: all 201 tests pass, and Ruff, formatting, pre-commit, and pyright are clean. I also tested the documented evaluation path and checkpoint-package behavior directly.

The remaining inline comments concern the current checkpoint publishing and loading contract, reproducibility metadata, spec creation, and documentation. One cross-cutting migration point also remains: the shared registry exposes 14 generators, but the default HF repositories currently exist for only cgan_cnn_2d, diffusion_2d_cond, gan_cnn_2d, and vqgan. Because this PR removes W&B checkpoint loading repository-wide, please either migrate the remaining registered generators, retain a fallback for the unmigrated ones, or explicitly narrow what the shared evaluator advertises as available. Issue #73 should remain open until the chosen scope is complete.

The README sentence saying that each algorithm usually has separate training and evaluation scripts should also be removed, since evaluation is now shared. I am treating the missing device override on Apple silicon as a follow-up rather than a blocker for this PR.

[SoheylM]

package_complete=False is currently checked only while constructing the message for missing files. If all required filenames are present, this function returns the package successfully even though it is explicitly marked incomplete. I reproduced that case locally. Please reject a package whenever metadata["package_complete"] is False, before file discovery, and add a test where the required files are present but the marker is false.

[mkeeler43]

Fixed in c0ea5ce.

_load_package_from_directory now tests metadata[PACKAGE_COMPLETE_FIELD] is False before file discovery and raises _incomplete_package_message. The branch inside _missing_files_message became unreachable once that landed, so I deleted it rather than leave dead defensive code — that function now only explains a finished package that is missing a file, which is a different problem and must not blame the run for stopping early.

Tests: test_an_incomplete_package_is_refused_even_with_every_file_present is exactly your case — every required filename present, marker false — plus test_a_complete_package_still_loads so the guard does not cost ordinary packages their ability to load.

I also checked what this rejects in practice, since it is a new hard failure: 143 published packages across engiopt-vqgan, engiopt-cgan-cnn-2d and engiopt-diffusion-2d-cond, and none is marked incomplete. So it breaks nothing that exists today.

[SoheylM]

This completeness marker can be defeated by the current upload behavior. upload_folder at line 474 overwrites matching files but does not remove files already present under package_path that are absent from stage_dir. A rerun or staged VQGAN upload can therefore retain weights from an earlier package generation. Combined with package discovery, this can produce a package containing files from different runs. Please replace or delete the existing contents of this package path as part of the upload. The promotion upload at line 419 needs the same treatment.

[mkeeler43]

Fixed in c0ea5ce, though not unconditionally — a blanket delete would have destroyed VQGAN's staging, which I think is worth spelling out since it is the case that makes this subtle.

VQGAN stage 2 uploads {vqgan.pth, discriminator.pth} and does not re-upload the cvqgan.pth that stage 1 wrote. delete_patterns="*" there would delete the earlier stage's work — the exact thing staged uploads exist to protect.

So the rule keys off completeness. _stale_file_patterns returns ["*"] when package_complete is True or absent (an ordinary single-stage model is complete and should not have to say so), and None for a stage that is deliberately additive. promote_to_canonical deletes unconditionally, because a promotion copies a whole package and must replace whatever held the canonical path before.

Tests: test_a_complete_upload_clears_whatever_was_at_that_path and test_a_staged_upload_leaves_the_earlier_stages_alone.

[SoheylM]

The recorded preprocessing state affects generated outputs but is excluded from content_hash because all of metadata.json is excluded. Changing condition_stats, condition_normalizer, or design_normalizer currently leaves the checkpoint hash unchanged; I verified this directly. That allows --skip-existing to reuse scores for behaviorally different packages. Please include a stable, identity-relevant subset of the metadata in the hash, while continuing to exclude mutable fields such as package path, revision, promotion history, and metrics.

[mkeeler43]

Fixed in c0ea5ce. hash_package_contents now folds identity_metadata(metadata) into the digest alongside the weight bytes.

One deliberate choice worth flagging, since you suggested naming an identity-relevant subset: I implemented it as a denylist, not an allowlist. ADDRESS_METADATA_FIELDS excludes package path, repo id, revision, promotion history and the W&B fields; everything else in metadata.json counts. An allowlist fails the dangerous way round — someone adds an output-affecting field, forgets to list it, and a behaviourally different checkpoint silently inherits an old score. A denylist's failure mode is that a bookkeeping field gets hashed and causes a harmless re-evaluation. I would rather pay that.

Tests: changing design_normalizer or condition_stats moves the hash; the same weights at the cfg and canonical paths still hash alike (they differ only in address fields); a promotion does not move it; and attaching metrics.json still does not, so the --skip-existing fixed point you and I both care about survives.

[SoheylM]

freeze_spec cannot set volume_condition or volfrac_tol, although these fields determine viol and the committed beams2d, heatconduction2d, and thermoelastic2d specs use a non-null volume condition. Running the documented CLI for a new version silently produces a different feasibility contract. Please expose both fields through this entry point and cover them in the CLI/spec tests.

[mkeeler43]

Fixed in c0ea5ce, and widened past the two fields you named — for the reason your comment implies. A parameter this entry point omits is a field it silently resets to the dataclass default, so the bug class is bigger than volume_condition and volfrac_tol.

Every field of the contract is now settable, including the four new ones this round adds (required_seeds, copy_tol, max_copy_rate, copy_corpus_size). Defaults reference EvalSpec.<field> rather than repeating literals.

You asked for test coverage and I had missed it — added in af6b018, thank you: test_freeze_spec_carries_every_contract_field asserts the values reach the frozen spec, and test_freeze_spec_defaults_track_the_dataclass asserts the CLI's defaults are the dataclass's, so the two cannot drift apart without a test failing.

[SoheylM]

This has the same parent-repository problem that was fixed for EngiBench: git -C searches upward, so an installed EngiOpt package inside another checkout can report that checkout's SHA as its own. Please use the same PEP 610/source-root validation approach here, or extract a shared helper, and test an installed-package path nested inside an unrelated repository.

[mkeeler43]

Fixed in c0ea5ce by extracting the shared helper, as you suggested.

_source_checkout_commit and _installed_vcs_commit are now one public source_checkout_commit(source_root, *, distribution=...) in spec.py, carrying both guards: an installed-package path is never a checkout, and the repository root must be the package's own parent rather than an ancestor. engibench_version() and code_version() both call it, so the two can no longer disagree about what counts as a checkout.

The existing tests moved to the shared helper and so now cover the EngiOpt path too: test_an_installed_package_is_never_read_as_a_checkout (a site-packages path) and test_a_directory_inside_an_unrelated_repo_is_not_that_repo (a real git init with the package nested below the root).

Observed live: rows now carry code_version: 0.0.1+af6b018..., which is this worktree rather than an enclosing checkout.

[SoheylM]

This exact command still fails because IDEALLab/engiopt-cgan-cnn-2d has no canonical beams2d/seed_1 package. The promotion example below also uses 023dd1fb with seed 1, but that configuration currently exists only for seed 42; a seed-1 configuration that exists is 825831f6. Please publish a canonical package and verify these commands verbatim, or document a configuration and seed that can currently be evaluated.

[mkeeler43]

Reproduced before fixing, and you are right on both counts — and the first one is worse than "no canonical package": IDEALLab/engiopt-cgan-cnn-2d holds 46 beams2d packages and not one canonical path.

Your 825831f6 is exactly right, and I can say why it is the only candidate: it is the sole cgan_cnn_2d configuration trained on seeds 1–10. The other 45 arms are seed 42 only, so promoting any of them to seed_1 would name a package that does not exist. Same shape for gan_cnn_2d, where the multi-seed arm is 6293adb3.

README now documents commands I ran verbatim:

• --generators cgan_cnn_2d --seeds 1 2 3 --config-fingerprints cgan_cnn_2d:825831f6, with a sentence on why the fingerprint is required rather than optional here.
• --generators diffusion_2d_cond --seeds 1 2 3 for the bare-seed case, since diffusion_2d_cond and vqgan do hold canonical packages. Ran it end to end: three rows, one entry, n_seeds=3.
• The promotion example now uses 825831f6.

I did not publish a canonical cgan_cnn_2d package — that is a checkpoint-publishing decision, and it overlaps the migration-scope question in your review summary, so it seemed like yours to make rather than mine.

[SoheylM]

This guidance suggests using conditions.dataset to drop columns and re-normalize as VQGAN did, but this PR correctly removed that behavior because evaluation-time refitting changes the model input scale. Please say that dataset is for conditions that cannot be represented in the dense tensor, while any training-fitted preprocessing must be recorded in the checkpoint and replayed. Also, viol is now cheap and runs without --include-expensive, so the feasibility wording here and in the evaluation CLI docstring should be updated.

[mkeeler43]

Rewritten in c0ea5ce; the viol half finished in af6b018 after I noticed I had only done part of it.

dataset. Now documented as being for conditions that cannot travel in the dense tensor at all — thermoelastic2d's 65x65 boundary matrices, which condition_keys excludes for that reason — with an explicit "do not use it to re-derive preprocessing" and the reason: the 50 evaluation rows have neither the training split's statistics nor the same set of columns that never vary. The replacement path is spelled out (condition_stats, condition_normalizer, design_normalizer recorded at save and replayed at load), plus the new fact that those values are part of the checkpoint content hash, so changing a bound yields a different checkpoint rather than inheriting the old row.

viol. You flagged two places and I had only fixed one. CONTRIBUTING now lists the cheap pass as mmd, dpp, viol, novelty, cond_sens and describes --include-expensive as adding only the gaps. The CLI docstring still said "COG/IOG/FOG, feasibility" until af6b018 — now corrected, with the reason feasibility is cheap: it describes the design as generated, so it still reports when the optimizer refuses to start from an invalid one.

[mkeeler43]

Round 2 — all seven comments answered, plus the change that lets this board be public

Threaded reply on each of the seven; short version below, with the two that
opened into something larger called out.

Comment	Fix
package_complete only checked while building the message	Checked before file discovery; the old branch became unreachable and was deleted rather than left as dead defensive code
upload_folder retains stale files	Deletes when the upload is the whole package, and deliberately not for a staged one — a blanket delete would have destroyed VQGAN's stage 1
Preprocessing excluded from content_hash	Folded in via identity_metadata, as a denylist so a future output-affecting field cannot be silently missed
freeze_spec cannot set volume_condition / volfrac_tol	Every contract field now settable; you asked for tests and I had missed them — added
code_version has the parent-repository bug	Shared source_checkout_commit(...) helper, used by both sides
README command still fails	Reproduced first; your 825831f6 confirmed and both commands now run verbatim
CONTRIBUTING conditions.dataset guidance + viol	Rewritten; the viol half was only half-done until af6b018

Two of these were symptoms of a bigger gap. A row recorded a checkpoint
hash but never a repo, and every default pointed at IDEALLab — so an
outside contributor could neither publish weights nor produce a row anyone could
trace. Every number was an assertion nobody could falsify. Rows now carry a full
address and python -m engiopt.verify re-runs from it; nothing is ranked until a
runner reproduces it. Separately, the protocol being public means a lookup table
scores mmd=0.000 and viol=0.00 on the real beams2d spec — better than every
trained model here — so the board now measures retrieval (novelty/copy_rate)
and condition-blindness (cond_sens) and declines to rank what trips them.

Full write-up in the PR description, and the trust model in
LEADERBOARD.md.

On the earlier threads

~21 threads from 3 and 6 Aug are still open in the UI. I re-verified each
against current code rather than assuming your summary covered them, and they
are all fixed — spot-checking the ones I trusted least: sweep YAMLs deleted, the
VQGAN adapter no longer refits on the evaluation sample, hash_package_contents
covers cvqgan.pth by listing the directory, local_model_dir reaches both
from_pretrained and the CLI, the device is synchronised before the sample timer
stops, and feasibility is computed independently of optimization. The two you
flagged as hidden by skips now genuinely run: 255 collected, 255 passed, zero
skipped. Happy to bulk-resolve those, or leave them for you.

Still open, and I don't think it's mine to close

Migration scope. Ten of fourteen registered generators have no published
checkpoint. --list-generators --check-availability now reports that per
generator instead of implying all fourteen are usable, but reporting is not
choosing between migrating them, keeping a fallback, and narrowing the registry.
#73 should stay open.

I also did not publish a canonical cgan_cnn_2d package. That is a
checkpoint-publishing decision that overlaps the above, so it seemed yours.

[SoheylM]

The public submission and verification boundary is not complete yet.

The documentation now describes a public leaderboard where outside contributors publish checkpoint-backed rows and an “official runner” later downloads and re-scores them. However, this PR does not define or deploy that runner: ideallab-ci is currently only a value stored in verified_by. There is no workflow, schedule, queue, or specified execution environment. We should not assume any particular future infrastructure.

More importantly, the submitted artifact is not yet safe for unattended execution. Verification reaches generator_cls.build(...), after which adapters deserialize participant-controlled .pth files using torch.load; VQGAN explicitly uses weights_only=False. The revision and content hash establish which bytes were evaluated, but they do not make those bytes safe to load.

I think this PR should establish a submission contract that a future official runner can consume safely, even if deploying that runner remains follow-up work:

• Require safetensors for new external submissions and reject pickle-based .pth/.pt files on the public path.
• Make EngiOpt export safetensors automatically so this does not become a manual participant step.
• Keep reconstruction settings and preprocessing state in validated JSON.
• Accept only registered EngiOpt adapters. New executable model code must still enter through a reviewed code PR.
• Before loading weights, validate the configuration and safetensors header against adapter-specific expectations: tensor names, shapes, supported dtypes, file size, tensor count and total element count.
• Pin the checkpoint repository revision and content hash, as this PR already does.
• Provide a submission mechanism that does not require IDEALLab write access. The current --push-to IDEALLab/engiopt-leaderboard performs a direct upload, despite LEADERBOARD.md saying outside contributors need no IDEALLab access. A pending manifest through a Hugging Face Community PR (create_pr=True) would be one straightforward option.
• Keep participant results verified=false; only the eventual official runner should publish verified scores.

Existing IDEALLab .pth checkpoints can temporarily use a separate, explicit allowlisted legacy path, with a follow-up issue to migrate them. A user-controlled repository or URL must not be able to select that path.

Isolation and runtime controls belong to the eventual runner: no credentials, restricted network access, and CPU/GPU, memory and time limits. Those controls do not have to be deployed in this PR, but they should be tracked before unattended verification is enabled.

Deploying and operating the official runner may remain follow-up work, but the safe submission contract cannot. Every external submission accepted after this PR should already contain safetensors weights, validated JSON configuration and preprocessing metadata, a registered adapter identifier, and an immutable repository revision and content hash. Participant-computed metrics may remain visible as verified=false until the official runner is available, but the submitted package must be directly consumable by that runner without conversion, migration, or resubmission.

[SoheylM]

README cleanup: The sentence “Usually, we provide two scripts per algorithm: one to train the model, and one to evaluate it” describes the architecture removed by this PR. Each generator still has its own training script and adapter, but evaluation is now handled centrally by python -m engiopt.evaluate. Could we replace it with: “Each generator provides its own training script and adapter; evaluation is handled through the shared python -m engiopt.evaluate command.”

[SoheylM]

The migration playbook is being removed before #73 meets its removal criteria.

Issue #73 says docs/checkpoint_migration_playbook.md should be deleted after the historical W&B checkpoint migration is complete. This PR deletes the file while leaving #73 open, and the PR description explicitly says migration scope remains unresolved.

I do not think this PR needs to migrate every historical checkpoint. However, could we either:

1. Restore and update the playbook for the new package layout, leaving Remove checkpoint migration playbook after W&B checkpoint migration is complete #73 open until migration is complete; or
2. Record an explicit decision not to migrate the remaining checkpoints, update the supported-generator documentation accordingly, and close or replace Remove checkpoint migration playbook after W&B checkpoint migration is complete #73.

Deleting the migration instructions while the migration decision remains open leaves no documented route for completing the work.

When documenting availability, please also distinguish missing checkpoints from incompatible generators. For beams2d, only seven registered generators support the problem kind; at the time of this review four have published checkpoints and three compatible families are missing. Counting all fourteen makes unrelated 1D/3D generators look like failed migrations.