Merge branch 'artifact-and-manifest-names' into 'main'

Name artifacts for their stage; rename the manifest (#17)

See merge request educelab/pgs-recon!71

Author: csparker247

.gitlab-ci.yml

@@ -52,6 +52,30 @@ test:python:
     - echo Running the test suite against the installed dependencies
     - python -m unittest discover -s tests -v
 
+# The only job that runs the suite where the binaries are. Both jobs above run
+# against a /usr/local with no OpenMVG in it, so anything reaching the built-in
+# default prefix passes for the wrong reason -- three tests did, asserting
+# ToolNotFound for real tool names and failing inside the image while CI stayed
+# green. The repo checkout shadows the image's editable install, so this tests the
+# MR's code against the real toolchain.
+#
+# It is also the only place test_openmvs.TestSurfaceMatchesTheInstalledBinaries
+# runs: OpenMVS is fetched as a tarball with no source tree to transcribe from, so
+# the binaries' own --help is the authority that ADR 0005's "every flag reachable"
+# is checked against, and that check skips itself wherever they are missing.
+test:in-image:
+  stage: test
+  image: ghcr.io/educelab/pgs-recon:edge
+  rules:
+    - if: $CI_PIPELINE_SOURCE == "merge_request_event"
+    - if: $CI_COMMIT_BRANCH
+  script:
+    # Assert the layout this job exists for, so a moved prefix fails here rather
+    # than quietly making the job meaningless again.
+    - test -x /usr/local/bin/openMVG_main_SfM
+    - test -x /usr/local/bin/OpenMVS/ReconstructMesh
+    - python -m unittest discover -s tests -v
+
 # Full C++ dependency build across the same base-image / CUDA matrix as the
 # GitHub Docker images. Expensive, so gated to MRs that actually touch the
 # dependencies, Dockerfile, or CI config. CUDA variants compile the .cu kernels

CLAUDE.md

@@ -35,13 +35,20 @@ The only tests are `tests/`: `python3 -m unittest discover -s tests`. They cover
 the staged-run planner (`test_stages.py`, pure logic, no filesystem), the stage
 records `StageTracker` writes (`test_tracker.py`, a temp dir but no binaries),
 artifact naming (`test_layout.py`), binary resolution and the run/record
-chokepoint (`test_toolchain.py`), `run_command`'s exit statuses
+chokepoint (`test_toolchain.py`), the MVS and MVG wrappers' flag surfaces
+(`test_openmvs.py` and `test_openmvg.py`, whose `SURFACES` tables are what make
+ADR 0005's "every flag reachable" enforceable — the MVG one maps each keyword
+argument to its argv flag, because OpenMVG's spellings are per-binary and cannot
+be derived), `run_command`'s exit statuses
 (`test_utility.py`), and `pgs-recon` end to end against a prefix of fake binaries
 plus its `--dry-run` (`test_pipeline.py`, `test_reconstruct.py`, which skip
 themselves when `configargparse`/`sfm_utils`/`exiftool` are missing). All
 stdlib-only, so they run anywhere in seconds — no reconstruction math is
 exercised, only what the pipeline asks the binaries to do. CI
-(`.gitlab-ci.yml`) runs that suite, then verifies that
+(`.gitlab-ci.yml`) runs that suite three ways — bare Python, with the Python deps
+installed, and inside `ghcr.io/educelab/pgs-recon:edge` (`test:in-image`, the only
+one where the binaries exist, so tests reaching the default prefix cannot pass for
+the wrong reason) — then verifies that
 dependencies build and the package pip-installs on Ubuntu 22.04 / 24.04. The
 canonical GitHub Actions workflow (`.github/workflows/build_docker.yml`)
 builds/publishes Docker images.
@@ -107,16 +114,18 @@ them and no `metadata` argument:
   that may legitimately be absent. Both are recorded paths read back from the
   manifest, never recomputed names.
 - **Outputs** are named by `pgs_recon/layout.py`, pure functions over the output
-  root (plus, until ADR 0006 lands, the input artifact a chained name derives
-  from). A wrapper never invents a filename; `layout` is the only place a name is
-  written, which is what makes renaming safe.
+  root: an intermediate is `<stage>_<role>.<ext>` and nothing in its name records
+  which other stages ran (ADR 0006). A wrapper never invents a filename; `layout`
+  is the only place a name is written, which is what makes renaming safe.
 - **Where the binaries are and what records them** is process-wide:
   `toolchain.configure(prefix=..., recorder=...)` once in `main()`.
   `toolchain.run()` is the single chokepoint that appends to
   `metadata['commands'][timestamp]` (a compatibility surface: `recon_dir.py` greps
   it) and then executes, so no wrapper can forget to record what it ran. An
-  `atexit` hook writes the manifest to `<output>/metadata.json`; the effective
-  config goes to `<output>/*_recon_config.txt`.
+  `atexit` hook writes the manifest to `<output>/pgs-recon.json`; the effective
+  config goes to `<output>/*_recon_config.txt`. `stages.find_manifest()` is what
+  reads it, falling back to a pre-1.8 `metadata.json` (ADR 0007) — the one
+  filesystem check in `stages.py`.
 
 When adding a stage: add a `layout` function for its output, add the wrapper as a
 pure argv builder ending in `run()`, and wire it in `run_pipeline` between
@@ -128,14 +137,24 @@ the wrappers stay a complete library surface over each binary's flags.
 ### Module layout
 
 - `pgs_recon/openmvg.py`, `pgs_recon/openmvs.py` — thin wrappers, one function per
-  binary, all routing through `toolchain.run`.
+  binary, all routing through `toolchain.run`. Each is its binary's *complete*
+  registered flag surface, transcribed from the pinned source and held to that by
+  the `SURFACES` tables above; when upstream moves, the wrapper and the table are
+  edited together. In `openmvg.py` a flag's spelling depends on how OpenMVG
+  registered it: `make_switch` flags are plain `bool` (no value exists to emit),
+  `make_option` over a bool is tri-state (`None` omits, `False` sends `0`), and
+  everything else is `None`-means-omit.
 - `pgs_recon/toolchain.py` — binary resolution, the run/record chokepoint, and
   what the binaries that resolve names against a directory need: `work_dir()`
   returns the directory a set of artifacts shares, refusing with
   `ArtifactsNotColocated` (a `ToolFailed`) when they disagree — OpenMVS really
   does require co-location. `relative_to_dir()` instead *translates*, for
   `openMVG_main_SfM`'s `-M`, whose join onto `-m` resolves `../`, so the matches
-  file may live anywhere; only an absolute path is unusable there.
+  file may live anywhere; only an absolute path is unusable there. `run()` is also
+  the one place argv becomes strings, so `_argv_token` narrows every `int`
+  subclass there rather than in each wrapper's flag builder — that is what makes a
+  `bool` reach argv as `0`/`1` and an `IntEnum` as its value (`str()` of one is its
+  member name before Python 3.11), including for flags a wrapper assembles by hand.
 - `pgs_recon/layout.py` — what every artifact of a run is called.
 - `pgs_recon/pgs_data.py` — import logic for EduceLab "PGS Scan" directories,
   including grid-scan neighbor lookup that generates an OpenMVG **view pairs file**
@@ -145,7 +164,7 @@ the wrappers stay a complete library surface over each binary's flags.
   and exits `128 + signum` for a signal death) and timestamp helper.
 - `pgs_recon/utils/` — shared helpers: `apps.py` (logging setup), `geometry.py`,
   `quality.py`, `charuco.py`, `wavefront.py`, `educelab.py` (ChArUco/board detection),
-  `recon_dir.py` (locate a finished run's SfM/mesh from its `metadata.json`),
+  `recon_dir.py` (locate a finished run's SfM/mesh from its manifest),
   `sfm_json.py` (OpenMVG SfM_Data JSON surgery: cereal polymorphic registration,
   extrinsic frame transforms), `images.py` (8-bit sRGB normalization).
 - `pgs_recon/apps/` — standalone CLI utilities (one `main()` each). Apps must NOT

CONTEXT.md

@@ -117,9 +117,12 @@ already independent of the shape keeps it, including every name a binary chooses
 for itself (`mvg/sfm_data.json`, `recon_dir/sfm_data.bin`) and the final
 deliverable `mvs/<name>.<ext>`, which is a user-facing contract. An artifact name
 is only ever *written* — a resumed job finds an existing artifact through the
-**manifest**, never by rebuilding its name. Not yet in effect: until
-[ADR 0006](./docs/adr/0006-stage-named-artifacts.md) lands, names chain off the
-input's stem and so *do* encode the shape (`scene_dense_refine.ply`).
+**manifest**, never by rebuilding its name
+([ADR 0006](./docs/adr/0006-stage-named-artifacts.md), which replaced the chained
+names an output directory used to carry: `scene_dense_refine.ply`). One stage
+cannot spell out two roles: `densify` hands `DensifyPointCloud` a single `-o`
+from which it names both files it writes, so the pair is `densify.mvs` /
+`densify.ply` — stem for the stage, suffix for the role.
 _Avoid_: path, key, stem, filename
 
 **Binding**:
@@ -140,7 +143,7 @@ than an error. Never inferred from what is on disk.
 _Avoid_: stale, invalid, out-of-date, needs-rebuild
 
 **Manifest**:
-The run's `metadata.json`, the single record of what a reconstruction has
+The run's `pgs-recon.json`, the single record of what a reconstruction has
 finished: each stage's status, the artifacts it produced, and the effective
 arguments it ran with. It is what makes a run resumable and what a later job
 consults instead of being told again.
@@ -197,7 +200,7 @@ Resulting layout (stem `IR940`, recon name `scroll`):
             IR940_scene.mvs
             IR940_input.ply              # staged copy of the mesh being textured
             IR940.obj                    # final, beside the recon's scroll.obj
-      IR940_retexture_metadata.json      # sidecar; recon's metadata.json untouched
+      IR940_retexture.json               # sidecar; recon's manifest untouched
       <datetime>_IR940_retexture_config.txt
 
 The only hard collision the convention removes is the **undistorted-images

README.md

@@ -28,8 +28,50 @@ docker run -v .:/working ghcr.io/educelab/pgs-recon:latest \
 Upon successful completion of the pipeline, your reconstructed model can be 
 found in `recon/mvs/my-object.obj`.
 
+### What lands in the output directory
+Every intermediate is named `<stage>_<role>`, after the stage that produced it,
+so a half-finished directory can be read for what has happened so far. Optional
+stages are marked; the rest are always present:
+
+```
+recon/
+  pgs-recon.json                  # the manifest: what ran, and with what arguments
+  my-object_recon_config.txt      # the effective arguments, loadable with -c
+  mvg/
+    sfm_data.json                 # the imported scene
+    matches_dir/                  # per-image features, matches[_filtered].bin
+    recon_dir/
+      sfm_data.bin                # the solve
+      robust_sfm.bin              # --mvg-robust
+      autoscale_sfm.bin           # --mvg-autoscale
+      landmarks[_scaled].ply      # --mvg-autoscale: the markers it scaled from
+      colorize_sfm.ply            # sparse cloud coloured from the images
+  mvs/
+    convert_scene.mvs             # the interface scene every MVS stage reads
+    undistorted_images/
+    densify.mvs  densify.ply      # --mvs-densify: scene + the dense cloud
+    reconstruct_mesh.ply
+    refine_mesh.ply               # --mvs-refine (on by default)
+    my-object.obj                 # the deliverable, + .mtl and texture image
+```
+
+**Locate an artifact through the manifest, not by rebuilding its name.** Every
+stage records the paths it consumed and produced, relative to the output
+directory, and those records are what a resumed job reads — which is what lets
+these names change without invalidating a directory that already exists:
+
+```shell
+jq -r '.stages.texture.outputs.mesh' recon/pgs-recon.json   # the textured mesh
+jq -r '.stages.convert.inputs.sfm'   recon/pgs-recon.json   # the solved SfM it came from
+```
+
+Upgrading from 1.7, where the manifest was `metadata.json` and intermediates were
+named by chaining (`scene_dense_refine.ply`)? Those directories are still read,
+but a 1.7 manifest carries no per-stage record, so a run against one rebuilds it
+from the start. See [docs/migrating-to-1.8.md](docs/migrating-to-1.8.md).
+
 ### Staged and resumable runs
-The pipeline records what it has finished in `<output>/metadata.json`, so
+The pipeline records what it has finished in `<output>/pgs-recon.json`, so
 **re-running the same command in the same output directory resumes it** rather
 than starting over. After a crash or an out-of-memory kill during mesh
 refinement, this picks up at `refine`:
@@ -75,19 +117,21 @@ mesh/refine/texture to a high-memory node, chained with `afterok`. Notes:
   re-refines and re-textures but touches nothing before it.
 * Changing the shape is allowed at any point. Adding `--mvs-densify` to a
   finished reconstruction re-runs densify and the mesh stages, and dropping it
-  again re-runs them against the non-dense filenames.
+  again re-runs them against the sparse cloud. Filenames stay put either way:
+  an artifact is named for the stage that wrote it, not for the stages upstream
+  of it.
 * Stages before `--from` are never run implicitly: if one is incomplete or its
   inputs have moved, the run fails immediately, naming each, instead of quietly
   doing work the job was not sized for.
 * If the range stops before stages the run invalidates, those stages are named
   in a warning and rebuilt by the next run that covers them. The final textured
   mesh keeps its usual `mvs/<name>.obj` filename in the meantime, so check the
   warning rather than the filename.
-* What is on disk is never consulted — `<output>/metadata.json` is the record. If
-  you delete an intermediate by hand, use `--rerun` to rebuild it.
+* What is on disk is never consulted — `<output>/pgs-recon.json` is the record.
+  If you delete an intermediate by hand, use `--rerun` to rebuild it.
 * An argument aimed at a stage outside the range is ignored with a warning,
-  because it would change filenames the rest of the pipeline has already
-  committed to. Per-invocation settings are exempt and can differ freely between
+  because it would change what the stages in range consume, and this run is not
+  sized to rebuild them. Per-invocation settings are exempt and can differ freely between
   jobs: `--path` and `--cam-db` apply silently, and `--threads`, `--log-level`,
   `--config` and `--output` are not recorded at all, so they never leak into a
   later job.
@@ -96,6 +140,30 @@ mesh/refine/texture to a high-memory node, chained with `afterok`. Notes:
 * `--no-mvs` is deprecated: use `--to colorize` for an SfM-only run. The old flag
   still works (it sets `--to colorize` and warns) but will be removed.
 
+### When mesh refinement takes too long
+`refine` is the pipeline's slowest and hungriest stage, and it can run out of two
+different resources. Out of **memory** is the familiar one, and resuming the same
+command picks up where the kill happened.
+
+Out of **wall clock** looks different: no progress in the log, one core pinned at
+100%, and memory flat. That is mesh *preparation* rather than the optimization —
+before refining anything, `RefineMesh` subdivides the input mesh and remeshes the
+result with single-threaded CGAL, which is silent at the default verbosity and on
+a mesh of a few hundred thousand vertices can run for tens of minutes or more.
+Adding cores or memory does not help. Turning it off does:
+
+```shell
+# Skip the remesh; refine everything else as before
+pgs-recon -o recon/ --from refine --refine-ensure-edge-size 0
+
+# Or subdivide less aggressively, so preparation has less to remesh
+pgs-recon -o recon/ --from refine --refine-max-face-area 64
+```
+
+Both change the refined mesh, so they are options rather than defaults. If refine
+is not worth its cost on a given dataset, `--no-mvs-refine` drops it from the
+pipeline shape and textures the reconstructed mesh directly.
+
 ### Docker images
 We provide multi-architecture (x86, arm64) Docker images in the 
 [GitHub Container Registry](https://github.com/educelab/pgs-recon/pkgs/container/pgs-recon).
@@ -154,7 +222,7 @@ transform. The input mesh must already be in the SfM coordinate frame.
 ```shell
 docker run -v .:/working ghcr.io/educelab/pgs-recon \
   pgs-sfm-orient \
-    -i /working/recon/sfm/sfm_data.bin \
+    -i /working/recon/mvg/recon_dir/sfm_data.bin \
     --input-mesh /working/recon/mvs/my-object.obj \
     -o /working/recon/mvs/my-object-centered.obj \
     --save-transform /working/recon/orient.npy \
@@ -283,8 +351,8 @@ so a typo in any of them is unambiguous. The OpenMVG camera sensor database is
 expected at `<prefix>/lib/openMVG/sensor_width_camera_database.txt`.
 
 Unlike most arguments, `--path` is deliberately *not* inherited from a previous
-run's `metadata.json` when a staged run resumes (see `--from`/`--to` above), so
-each job of a split reconstruction picks up the prefix of the node it lands on.
+run's manifest when a staged run resumes (see `--from`/`--to` above), so each job
+of a split reconstruction picks up the prefix of the node it lands on.
 
 ### Advanced Installation
 #### Installation Location

apptainer/submit_recon_pipeline.sh

@@ -14,7 +14,7 @@
 # after that work is already done. Here only job 3 asks for the big node.
 #
 # Each job runs `pgs-recon` against the same --output directory and picks up
-# where the last stopped; state lives in that directory's metadata.json. Only
+# where the last stopped; state lives in that directory's pgs-recon.json. Only
 # job 1 needs -i/--name/-c: every argument it ran with is recorded and reloaded
 # by the later jobs.
 #
@@ -106,14 +106,15 @@ sbatch_common=(
 
 # --- Job 1: stage in, convert to JPG, run every OpenMVG stage ----------------
 # --to convert stops after openMVG2openMVS, so this job produces the interface
-# scene.mvs that densify consumes and nothing more.
+# convert_scene.mvs that densify consumes and nothing more.
 #
-# --mvs-densify belongs HERE, on the run that establishes the pipeline shape:
-# densify renames the whole mesh chain (scene_mesh.ply -> scene_dense_mesh.ply),
-# so adding it on a later job would invalidate stages that job is not sized to
-# rebuild. pgs-recon would notice and warn rather than silently skip them, but
-# the chain would still need a re-run to catch up. (--mvs-refine is on by
-# default, so refine is already in the shape.)
+# --mvs-densify belongs HERE, on the run that establishes the pipeline shape.
+# Artifact names no longer move when it is added (ADR 0006), but reconstruct's
+# input still does -- from convert_scene.mvs to densify.mvs -- so adding it on a
+# later job would invalidate stages that job is not sized to rebuild. pgs-recon
+# would notice and warn rather than silently skip them, but the mesh stages
+# would still need a re-run to catch up. (--mvs-refine is on by default, so
+# refine is already in the shape.)
 job1=$(sbatch --parsable "${sbatch_common[@]}" \
   --job-name="pgs-mvg-${job_name}" \
   --output="pgs-recon_mvg_%j_out.txt" \
@@ -177,6 +178,12 @@ printf '  job 2  %-22s %s  %s\n' "densify" "${part_gpu}" "${job2}"
 # recorded complete and skipped. To retry refine less aggressively instead, add
 # an argument it owns and the stages downstream of it re-run too:
 #   pgs-recon -o <dir> --from refine --refine-resolution-level 2
+#
+# Memory is not the only way this job runs out. At the pinned OpenMVS, refine's
+# mesh *preparation* remeshes with single-threaded CGAL before any optimization
+# starts, and on a large mesh that can burn hours on one core with flat RSS -- so
+# a timeout here means --time or the knobs below, not --mem:
+#   pgs-recon -o <dir> --from refine --refine-ensure-edge-size 0
 job3=$(sbatch --parsable "${sbatch_common[@]}" \
   --job-name="pgs-mesh-${job_name}" \
   --output="pgs-recon_mesh_%j_out.txt" \
@@ -233,7 +240,7 @@ time tar -cvzf "${processed_job_dir}/intermediate.tar.gz" \\
   "${processed_job_dir}/jpg" \\
   "${processed_job_dir}/mvg" \\
   "${processed_job_dir}/mvs" \\
-  "${processed_job_dir}/metadata.json" \\
+  "${processed_job_dir}/pgs-recon.json" \\
   ${processed_job_dir}/*.txt \\
   --remove-files