GMTSAR S1 backend trial#4
Open
dunyuliu wants to merge 13 commits into
Open
Conversation
…ssing New processor/gmtsar_s1.py + GMTSAR_S1_Config, implementing the real LocalProcessor interface (submit/refresh/retry/watch/save), matching ISCE_S1's conventions (status constants, job JSON, per-pair concurrent execution via ThreadPoolExecutor instead of ISCE_S1's per-step model, since p2p_processing is one-pair-per-invocation). Verified against real sources: p2p_processing's actual CLI contract (SAT ref sec [config.py]), and MintPy's own prep_gmtsar.py -- confirmed it consumes GMTSAR's native intf/<ref>_<sec>/ output layout directly, so no output-normalization step is needed before a Mintpy analyzer can read a GMTSAR_S1 case. Real-tested in this repo (not just import-shimmed against the public GMTSAR_S1 docstring): imports cleanly, registers in the real Processor registry, constructs correctly, and correctly raises NotImplementedError for the one genuinely-unimplemented path (bbox-driven DEM auto-fetch -- GMTSAR has no GLO-30-style auto-download equivalent to ISCE_S1 yet) instead of silently pretending to work. Known gaps before this is real, tracked for the next pass: - Whether p2p_processing internally preprocesses raw .SAFE/.zip input or expects already-focused SLCs (biggest open unknown, needs tracing p2p_stages.py's stage 1 against real S1 data). - DEM auto-fetch (dem_path=None currently raises, doesn't auto-download). - No HPC/SLURM support yet (ISCE_S1 has a real hpc_mode path; this is local ThreadPoolExecutor only, deliberately out of scope for v0). - Never run against real Sentinel-1 data end-to-end yet. Deliberately kept as subprocess-per-pair (not in-process calls into GMTSAR's own p2p_stages.py): separate conda environments with different numpy/GDAL stacks make in-process imports risky, most wall-clock is in C binaries either way so it wouldn't be faster, and this matches both ISCE_S1's own external-process pattern and GMTSAR's own test harness design (case_runner.py deliberately isolates each case in its own subprocess).
…irs signature bug found Traced p2p_stages.py::P2P1Preprocess directly: it calls pre_proc internally on raw/ input, resolving the README's 'biggest open unknown' (_stage_case()'s raw-staging assumption was correct). Found a real bug while tracing this: GMTSAR_S1's pairs=[(ref,sec),...] signature copied ISCE_S1's plain-date-tuple convention, but S1_TOPS's actual p2p_processing master/aligned args are raw per-subswath product basename stems, not YYYYMMDD dates (confirmed against a real recipe, tests/recipes/README_S1_Ridgecrest_EQ.txt in the GMTSAR repo). Also found a second, different entry point (p2p_S1_TOPS_Frame) for multi-subswath cases with its own distinct signature. Not fixing the signature yet -- it's a real design decision (which S1 variant to target, or both) that needs a human call, not a unilateral autonomous rewrite of the public interface. Documented in docs/gmtsar_s1_notes/OPEN_ISSUES.md for the next session.
Two real bugs found and fixed via actual end-to-end testing against real cached S1_Ridgecrest_EQ data, not just code review: 1. pairs=[(ref,sec),...] copied ISCE_S1's date-tuple convention, but GMTSAR's real S1_TOPS args are raw per-subswath product stems, not YYYYMMDD dates -- confirmed against a real recipe. Now supports BOTH real GMTSAR entry points via config.frame_mode: single-subswath p2p_processing (default) and multi-subswath p2p_S1_TOPS_Frame. _build_cmd() output verified identical to the real recipe lines for both modes, not just reviewed. 2. Real end-to-end run revealed GMTSAR's own Python stages (e.g. dem2topo_ra) shell out to the standalone `gmt` binary, which comes from GMTSAR's own conda env, not from GMTSAR's bin/ dir. InSARHub runs in a separate conda env without `gmt` on PATH at all -- confirmed directly (`which gmt`: not found in InSARHub's env). Without this fix every GMTSAR subprocess call failed in ~1s with no useful error surfaced. Fixed: GMTSAR_S1_Config gained gmtsar_root / gmtsar_env_bin; _subprocess_env() builds an explicit PATH for every GMTSAR subprocess call, independent of the caller's own environment. Also hardened _stage_case() against a real broken symlink loop found in real GMTSAR test data (a stray self-referencing .EOF) -- now skipped with a warning instead of crashing staging entirely. A real end-to-end validation run (Frame mode, with the env fix) is in progress in the background as of this commit -- see docs/gmtsar_s1_notes/OPEN_ISSUES.md for status; do not trust this as "done" until that lands.
Real run against S1_Ridgecrest_EQ data completed rc=0 through preprocessing, per-subswath SLC/topo/intf/filter, merge, unwrap, and geocode, producing real phasefilt_ll.grd/corr_ll.grd products. proc.watch() reported SUCCEEDED. Also documents the remaining known gap: single-subswath mode is not yet fixed for the H_res per-subswath extraction step.
…t config validation Both frame_mode settings now take the same pairs = [(ref_safe, ref_eof, sec_safe, sec_eof), ...] shape. Single-subswath mode (frame_mode=False) previously required callers to hand-derive raw per-subswath product stems themselves, which was correct per GMTSAR's CLI contract but bad UX and untested. GMTSAR_S1 now does that extraction itself (_extract_subswath_stem), mirroring GMTSAR's own H_res/raw/ single- subswath test fixture layout. Both modes are now real end-to-end validated against real Sentinel-1 data (see OPEN_ISSUES.md). Audit fixes: dem_path/gmtsar_root/gmtsar_env_bin are now validated at construction time instead of failing deep inside a background staging thread; _status_dir() no longer raises a masking KeyError for pairs that were never staged (returns a not-yet-existing path, reading as PENDING); added test coverage for the FAILED (nonzero returncode) path and frame_mode=True staging via submit(), both previously unguarded. Also: added CHANGELOG.md entry, GMTSAR_S1_Config docstring gap fix, GMTSAR_S1 docs tab in docs/advanced/processor.md matching the existing ISCE_S1/Hyp3_S1 pattern.
…ath mode
Real bug found via MintPy integration testing: p2p_processing's actual
output directory is named by GMTSAR's own Julian-date pair (e.g.
intf/2019184_2019196/, derived from each SLC's SC_clock_start), NOT
intf/<ref_stem>_<sec_stem>/ as the code and docs previously claimed. The
wrong assumption was masked because our own status-marker write mkdir'd
its own directory regardless of whether GMTSAR actually wrote output
there, so SUCCEEDED still reported correctly -- but downstream consumers
(MintPy) pointed at the documented path would find nothing, since
prep_gmtsar.py derives DATE12 by parsing the real Julian-date dirname.
_run_one_pair() now diffs intf/'s contents before/after each real run
and records whichever new directory matches GMTSAR's \d{7}_\d{7} naming;
_status_dir() uses that discovered directory once known, falling back to
a stem-based sentinel (which can never collide with a real directory)
before a run completes. Updated all docstring/doc claims accordingly and
added test coverage for both the pre-run and post-run status_dir paths.
Confirmed against a real single-subswath run with unwrapping enabled
(threshold_snaphu=0.1): real unwrap_ll.grd landed under the correct
Julian-date directory this fix discovers.
Found via real MintPy integration testing: gdal.Open() on GMTSAR's .grd (netCDF) output returned None in the insarhub_test env (pip- installed GDAL, missing the netCDF driver plugin) -- prep_gmtsar.py crashed with AttributeError before reaching any real logic. Pinning libgdal-netcdf explicitly (rather than assuming gdal >=3.8 always bundles it) makes this reproducible for anyone rebuilding the env.
Confirmed real fixes for ALOOKS/RLOOKS, HEADING, and the missing GDAL netCDF driver against real single-subswath output. Documents the real remaining blocker: read_baseline_table() needs a stack-level baseline_table.dat across multiple SLC dates, which our one-pair validation run doesn't produce. Not fabricated -- logged as an open item pending either a real multi-pair run or an explicit decision to build a synthetic stand-in.
… reload
Real CLI gaps found by actually running GMTSAR_S1 through the CLI
(refresh against real saved job state), not just reading source:
- _find_jobs_file() hardcoded "isce_jobs*.json" + an ISCE-only
subdirectory lookup at all 4 call sites (refresh/retry/cancel/watch),
so it could never find GMTSAR_S1's gmtsar_jobs.json. Added
JOBS_FILE/JOBS_SUBDIR class attributes to LocalProcessor (set on
ISCE_Base and GMTSAR_S1) and a _jobs_glob() helper deriving the
search pattern/subdir generically per processor.
- _proc_local_submit hardcoded 2-tuple pairs ((p[0], p[1])), silently
truncating GMTSAR_S1's required 4-tuples. Now preserves full arity.
- _load_local_processor's saved-job reload assumed ISCE's
{"jobs": {...}} wrapper and step-based pairs ((j["step"], j["step"])).
GMTSAR_S1.save() writes the jobs dict directly at the top level and
stores the real pair under "pair" -- now handles both shapes.
Confirmed via a real CLI refresh against GMTSAR_S1's actual saved
gmtsar_jobs.json: discovery, pairs-arity, and reload-shape now all work
end-to-end. Surfaced the next real gap in the process: GMTSAR_S1's
refresh()/watch()/cancel() don't share ISCE_Base's method signatures
(ls=, refresh_interval=, cancel() doesn't exist) -- not fixed here,
logged as a follow-up.
…ing reload Real gaps found by actually running refresh/watch/cancel against GMTSAR_S1 through the CLI as separate process invocations (the normal CLI usage pattern -- submit in one call, refresh/watch/cancel later in another): - cli/main.py: refresh(ls=...)/watch(refresh_interval=...) assumed every local processor shares ISCE_Base's exact method signature; crashed on GMTSAR_S1. Added _call_if_supported() -- calls each processor's method with only the kwargs its real signature (via inspect.signature) accepts. cancel() has no GMTSAR_S1 equivalent at all; now a clean error instead of AttributeError. - gmtsar_s1.py: a freshly CLI-reconstructed GMTSAR_S1 had self.jobs permanently empty (refresh() only updates pre-existing entries), so a reloaded processor printed nothing regardless of real on-disk status. For frame_mode=False this went deeper: self._stems/_real_intf_dirs are populated only in-memory during the ORIGINAL submitting process, never persisted. Fixed with read-only rediscovery at construction time: _rediscover_stem() (matches already-extracted raw/*.tiff symlinks against a pair's real .SAFE name) and _rediscover_real_intf_dir() (recomputes the Julian-date intf/ dirname from real SC_clock_start in the already-present .PRM files, same formula _run_one_pair() uses live). frame_mode=True needed none of this -- its _status_dir() is purely path-based. Confirmed via real CLI refresh/watch/cancel runs against the actual completed single-subswath job, in a fresh process with no submit() call: refresh and watch both correctly report SUCCEEDED; cancel correctly refuses instead of crashing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GMTSAT S1 backend is developed in InSAR Hub workflow style. First trial.