feat(brn-005,brn-006): pre-BPE RINEX validator + RUNX_v2 headless mode - #37
Closed
alfieprojectsdev wants to merge 6 commits into
Closed
feat(brn-005,brn-006): pre-BPE RINEX validator + RUNX_v2 headless mode#37alfieprojectsdev wants to merge 6 commits into
alfieprojectsdev wants to merge 6 commits into
Conversation
Staff flagged RXOBV3 silent station drops as the top Bernese pain point. BRN-006 specifies a pre-flight validator that cross-checks RINEX REC/ANT headers against the STA TYPE 002 and staged ATX file before any BPE run.
…s-check Staff-identified bottleneck: RXOBV3 (PID 221/222) silently drops stations whose RINEX REC TYPE / ANT TYPE headers don't match the campaign STA file. With ~270 stations and inconsistent metadata entry, this causes repeated surprise station losses only discovered after examining the solution. - rinex_header_validator.py: parses RINEX 2/3 OBS headers (80-col fixed width), extracts REC TYPE (cols 20-39 of REC # / TYPE / VERS record) and ANT TYPE (cols 20-39 of ANT # / TYPE record) per station - Reads STA TYPE 002 at known fixed offsets: receiver [69:89], antenna [121:141], station code [0:4] - Optionally cross-checks antenna type against staged ATX calibration file - Antenna comparison tolerates radome suffix omission in STA (e.g. "LEIAR20 NONE" header vs "LEIAR20" STA entry — treated as match) - ValidationReport.ok is False if any mismatch, missing-from-STA, or ATX-absent case is found; ValidationError carries the full mismatch list - LinuxBPEBackend.run() calls the validator as a pre-flight check before invoking the BPE Perl script; skips gracefully if RAW/ or STA not present - 21 tests covering: RINEX parsing, STA parsing, ATX parsing, radome tolerance, match/mismatch cases, missing-from-STA, ATX coverage, ValidationError message, and backend integration
…peline hook RUNX_v2.py had three input() gates blocking headless execution: 1. getxyz(): "Press Enter to continue" 2. transform(): "Input reference station:" (the critical one) 3. plotfiles(): "Press Enter" + time.sleep(3) pause Changes: - start() now accepts reference_station parameter; interactive=True when it is None, preserving the existing staff workflow unchanged - All input() and time.sleep() calls gated on interactive flag - Added argparse with --reference-station / -r; guarded by __name__ == '__main__' so the module remains importable - BerneseOrchestrator.run_velocity_pipeline(): invokes the script headlessly via subprocess from a given crd_dir, passes --reference-station through; raises RuntimeError on non-zero exit - outlier_input-site.py is intentionally NOT automated per ticket spec; that gate is replaced by the velocity-reviewer web UI
📝 WalkthroughWalkthroughThis PR adds a pre‑BPE RINEX header validator (BRN-006) integrated as a pre‑flight gate in the Bernese backend, implements headless/CLI execution for the RUNX velocity pipeline with an optional reference station, and includes parsing, data structures, tests, and backlog documentation. ChangesRUNX Headless + RINEX Header Validation
Sequence DiagramsequenceDiagram
actor User
participant Backend as LinuxBPEBackend
participant Validator as validate_rinex_headers
participant RAW as RAW/ (RINEX files)
participant STA as .STA file
participant ATX as ATM/*.atx
User->>Backend: run()
activate Backend
alt RAW/ and .STA present
Backend->>Validator: validate_rinex_headers(RAW, STA, atx_candidate)
activate Validator
Validator->>RAW: parse RINEX headers
RAW-->>Validator: station → receiver/antenna
Validator->>STA: parse TYPE 002
STA-->>Validator: station → expected receiver/antenna
opt ATX candidate found
Validator->>ATX: parse antenna types
ATX-->>Validator: antenna types set
end
Validator->>Validator: compare headers ↔ STA (ant radome tolerance)
Validator-->>Backend: ValidationReport (ok / mismatches / atx_missing)
deactivate Validator
alt report.ok
Backend->>Backend: continue run
else
Backend-->>User: raise ValidationError
end
else
Backend->>Backend: skip pre-flight check
end
deactivate Backend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
services/bernese-workflow/tests/test_rinex_header_validator.py (1)
389-391: 💤 Low valueConsider patching at the usage location for robustness.
The patch target
"subprocess.run"works, but patching at the import location ("bernese_workflow.backends.subprocess.run") is more robust against import restructuring.♻️ Suggested change
- with patch("subprocess.run") as mock_run: + with patch("bernese_workflow.backends.subprocess.run") as mock_run:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/bernese-workflow/tests/test_rinex_header_validator.py` around lines 389 - 391, The test currently patches subprocess.run globally; change the patch target to the import location used by the code under test by replacing patch("subprocess.run") with patch("bernese_workflow.backends.subprocess.run") so the mocked call applies where backend.run (the function invoked as result = backend.run("NORAW", 2023, "0100")) actually calls subprocess.run; keep the same MagicMock return_value setup for stdout/stderr/returncode.services/bernese-workflow/src/bernese_workflow/orchestrator.py (1)
75-86: 💤 Low valueConsider validating
runx_scriptexists before invoking subprocess.If
runx_scriptdoesn't exist, the subprocess will fail with a generic Python error. Adding an upfront existence check would provide a clearer error message.♻️ Proposed validation
crd_dir = Path(crd_dir) runx_script = Path(runx_script) + + if not runx_script.exists(): + raise FileNotFoundError(f"RUNX_v2 script not found: {runx_script}") logger.info(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py` around lines 75 - 86, The code calls subprocess.run with runx_script (converted via Path(runx_script)) without verifying the file exists; add an existence and executable check before invoking subprocess.run in the function where crd_dir and runx_script are handled: convert runx_script to a Path, check runx_script.exists() (and optionally runx_script.is_file() and os.access(runx_script, os.X_OK)), and if the check fails log a clear error via logger.error (including the runx_script path) and raise a descriptive exception or return early instead of calling subprocess.run; keep the existing logger.info and subprocess.run usage otherwise.services/bernese-workflow/src/bernese_workflow/rinex_header_validator.py (1)
130-138: 💤 Low valueMultiple RINEX files for the same station silently overwrites.
If
RAW/contains multiple RINEX files with the same station code (e.g., different days), the last-processed file's headers will silently overwrite earlier ones. If headers differ between files for the same station, mismatches could go undetected.This may be acceptable if campaigns are expected to have consistent headers per station, but consider logging when a station code is seen multiple times.
services/bernese-workflow/src/bernese_workflow/backends.py (1)
176-178: ⚡ Quick winMultiple ATX files silently skips ATX validation.
When
ATM/contains more than one.atx/.ATXfile, the ATX coverage check is silently skipped (atx_path = None). This could hide configuration issues or stale ATX files.Consider logging a warning when multiple ATX candidates are found, or raising an error to force explicit ATX selection.
♻️ Proposed fix
atm_dir = campaign_path / "ATM" atx_candidates = sorted(atm_dir.glob("*.atx")) + sorted(atm_dir.glob("*.ATX")) if atm_dir.exists() else [] - atx_path = atx_candidates[0] if len(atx_candidates) == 1 else None + if len(atx_candidates) == 1: + atx_path = atx_candidates[0] + elif len(atx_candidates) > 1: + logger.warning( + "Multiple ATX files found in %s; skipping ATX coverage check. " + "Remove stale files or specify explicitly.", + atm_dir, + ) + atx_path = None + else: + atx_path = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/bernese-workflow/src/bernese_workflow/backends.py` around lines 176 - 178, The current logic in backends.py sets atx_path = None when more than one ATX candidate is found, silently skipping validation; update the code that builds atm_dir, atx_candidates and atx_path (using campaign_path, atm_dir, atx_candidates, atx_path) to explicitly handle multiple candidates by either raising an exception or emitting a clear warning (processLogger.warn/process_logger.warning) that lists the found files and instructs the user to pick one, and only set atx_path when exactly one candidate exists; ensure the new branch returns/propagates the error or warning so ATX validation is not silently skipped.analysis/02 Time Series/RUNX_v2.py (1)
28-37: ⚖️ Poor tradeoffFile handles are not closed properly.
Multiple places use
open()without a context manager (withstatement), which can leak file handles if exceptions occur. This pattern repeats at lines 28-30, 35-37, 71, 78-79, 95-96, 120-122, and 144-148.♻️ Example fix for one instance
if len(line.split()) == 7: - f = open('XYZ', 'a+') - f.write('{:.4s} {:.5s} {:>13} {:>13} {:>13}\n'.format(x[1], files[2:7], x[3], x[4], x[5])) - f.close() + with open('XYZ', 'a+') as f: + f.write('{:.4s} {:.5s} {:>13} {:>13} {:>13}\n'.format(x[1], files[2:7], x[3], x[4], x[5]))Given this is existing code being minimally modified for headless support, this refactor can be deferred.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@analysis/02` Time Series/RUNX_v2.py around lines 28 - 37, Replace the manual open()/write()/close() pattern with context managers: use "with open('XYZ', 'a+') as f:" around the write calls that currently use variable f (the blocks that write the formatted line using '{:.4s} {:.5s} ...'.format(...), the separator line write('----------------------------------------------------------\n'), and any other write sites guarded by the elif len(line.split()) checks). Ensure the f.write calls remain unchanged but remove explicit f.close(), so file handles are closed automatically even on exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/bernese-workflow/src/bernese_workflow/backends.py`:
- Line 177: The glob for atx files can produce duplicates on case-insensitive
filesystems because both "*.atx" and "*.ATX" match the same file; update the
assembly that builds atx_candidates (the atm_dir.glob calls that feed
atx_candidates) to deduplicate case-insensitively by using each
Path.name.lower() as the key (retain the first Path encountered) so you end up
with a list of unique Path objects; keep references to atm_dir and
atx_candidates when implementing the de-dup logic so the rest of the function
can remain unchanged.
---
Nitpick comments:
In `@analysis/02` Time Series/RUNX_v2.py:
- Around line 28-37: Replace the manual open()/write()/close() pattern with
context managers: use "with open('XYZ', 'a+') as f:" around the write calls that
currently use variable f (the blocks that write the formatted line using '{:.4s}
{:.5s} ...'.format(...), the separator line
write('----------------------------------------------------------\n'), and any
other write sites guarded by the elif len(line.split()) checks). Ensure the
f.write calls remain unchanged but remove explicit f.close(), so file handles
are closed automatically even on exceptions.
In `@services/bernese-workflow/src/bernese_workflow/backends.py`:
- Around line 176-178: The current logic in backends.py sets atx_path = None
when more than one ATX candidate is found, silently skipping validation; update
the code that builds atm_dir, atx_candidates and atx_path (using campaign_path,
atm_dir, atx_candidates, atx_path) to explicitly handle multiple candidates by
either raising an exception or emitting a clear warning
(processLogger.warn/process_logger.warning) that lists the found files and
instructs the user to pick one, and only set atx_path when exactly one candidate
exists; ensure the new branch returns/propagates the error or warning so ATX
validation is not silently skipped.
In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py`:
- Around line 75-86: The code calls subprocess.run with runx_script (converted
via Path(runx_script)) without verifying the file exists; add an existence and
executable check before invoking subprocess.run in the function where crd_dir
and runx_script are handled: convert runx_script to a Path, check
runx_script.exists() (and optionally runx_script.is_file() and
os.access(runx_script, os.X_OK)), and if the check fails log a clear error via
logger.error (including the runx_script path) and raise a descriptive exception
or return early instead of calling subprocess.run; keep the existing logger.info
and subprocess.run usage otherwise.
In `@services/bernese-workflow/tests/test_rinex_header_validator.py`:
- Around line 389-391: The test currently patches subprocess.run globally;
change the patch target to the import location used by the code under test by
replacing patch("subprocess.run") with
patch("bernese_workflow.backends.subprocess.run") so the mocked call applies
where backend.run (the function invoked as result = backend.run("NORAW", 2023,
"0100")) actually calls subprocess.run; keep the same MagicMock return_value
setup for stdout/stderr/returncode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 64ccf195-dcfc-4066-8ee3-296583c051bf
📒 Files selected for processing (6)
analysis/02 Time Series/RUNX_v2.pydocs/project_documentation/ticket_backlog.mdservices/bernese-workflow/src/bernese_workflow/backends.pyservices/bernese-workflow/src/bernese_workflow/orchestrator.pyservices/bernese-workflow/src/bernese_workflow/rinex_header_validator.pyservices/bernese-workflow/tests/test_rinex_header_validator.py
- backends.py: deduplicate ATX candidates via resolve() to handle case-insensitive filesystems (macOS); add explicit warning log when multiple ATX files are found in ATM/ instead of silently skipping ATX coverage check - orchestrator.py: raise FileNotFoundError before subprocess.run if runx_script does not exist, providing a clear message rather than a generic Python traceback - test: patch "bernese_workflow.backends.subprocess.run" at the import location rather than the global "subprocess.run" for robustness against import restructuring Skipped: RUNX_v2.py file-handle refactor (CodeRabbit noted "can be deferred" — pre-existing pattern outside this PR's scope)
alfieprojectsdev
added a commit
that referenced
this pull request
May 5, 2026
- backends.py: deduplicate ATX candidates via resolve() to handle case-insensitive filesystems (macOS); add explicit warning log when multiple ATX files are found in ATM/ instead of silently skipping ATX coverage check - orchestrator.py: raise FileNotFoundError before subprocess.run if runx_script does not exist, providing a clear message rather than a generic Python traceback - test: patch "bernese_workflow.backends.subprocess.run" at the import location rather than the global "subprocess.run" for robustness against import restructuring Skipped: RUNX_v2.py file-handle refactor (CodeRabbit noted "can be deferred" — pre-existing pattern outside this PR's scope)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/bernese-workflow/src/bernese_workflow/orchestrator.py (1)
58-59: ⚡ Quick winEnforce the documented 4-character station-code contract.
Line 66 documents a 4-char code, but the method accepts any string. Validating/sanitizing early gives clearer failures than delegating this to RUNX_v2 runtime behavior.
Suggested patch
def run_velocity_pipeline( self, reference_station: str, @@ - logger.info( - "Running velocity pipeline: ref=%s crd_dir=%s", reference_station, crd_dir - ) + reference_station = reference_station.strip().upper() + if len(reference_station) != 4 or not reference_station.isalnum(): + raise ValueError( + f"Invalid reference_station '{reference_station}'; expected a 4-character station code." + ) + + logger.info("Running velocity pipeline: ref=%s crd_dir=%s", reference_station, crd_dir)Also applies to: 82-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py` around lines 58 - 59, Validate the station-code parameters early where reference_station: str is declared: ensure reference_station (and the other station-code parameters in the same signature block around lines 82–87) are exactly four characters (and optionally restrict to allowed charset such as ASCII letters/digits) and raise a clear ValueError with a descriptive message if the check fails; perform this check at the start of the function/method that declares reference_station so invalid values fail fast and do not reach RUNX_v2.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py`:
- Around line 76-77: crd_dir is converted to Path but not validated before being
passed as cwd to subprocess.run, so add an explicit check that crd_dir.exists()
and crd_dir.is_dir() (similar to the existing runx_script guard) and raise a
clear error (or log and exit) if it fails; update the logic around the
Path(crd_dir) conversion and before the subprocess.run call that uses
cwd=crd_dir to ensure the directory is present and readable, referencing the
crd_dir variable and the subprocess invocation that sets cwd.
---
Nitpick comments:
In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py`:
- Around line 58-59: Validate the station-code parameters early where
reference_station: str is declared: ensure reference_station (and the other
station-code parameters in the same signature block around lines 82–87) are
exactly four characters (and optionally restrict to allowed charset such as
ASCII letters/digits) and raise a clear ValueError with a descriptive message if
the check fails; perform this check at the start of the function/method that
declares reference_station so invalid values fail fast and do not reach RUNX_v2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20768b65-2cde-4400-8a22-0b35bcffd47a
📒 Files selected for processing (3)
services/bernese-workflow/src/bernese_workflow/backends.pyservices/bernese-workflow/src/bernese_workflow/orchestrator.pyservices/bernese-workflow/tests/test_rinex_header_validator.py
✅ Files skipped from review due to trivial changes (1)
- services/bernese-workflow/src/bernese_workflow/backends.py
🚧 Files skipped from review as they are similar to previous changes (1)
- services/bernese-workflow/tests/test_rinex_header_validator.py
Comment on lines
+76
to
+77
| crd_dir = Path(crd_dir) | ||
| runx_script = Path(runx_script) |
There was a problem hiding this comment.
Pre-validate crd_dir before subprocess launch.
Line 87 relies on cwd=crd_dir, but crd_dir is never validated. If it is missing/not a directory, the failure bubbles up from subprocess.run with a less explicit error path than your runx_script guard.
Suggested patch
crd_dir = Path(crd_dir)
runx_script = Path(runx_script)
+ if not crd_dir.is_dir():
+ raise FileNotFoundError(f"CRD directory not found: {crd_dir}")
+
if not runx_script.is_file():
raise FileNotFoundError(f"RUNX_v2 script not found: {runx_script}")Also applies to: 85-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py` around lines
76 - 77, crd_dir is converted to Path but not validated before being passed as
cwd to subprocess.run, so add an explicit check that crd_dir.exists() and
crd_dir.is_dir() (similar to the existing runx_script guard) and raise a clear
error (or log and exit) if it fails; update the logic around the Path(crd_dir)
conversion and before the subprocess.run call that uses cwd=crd_dir to ensure
the directory is present and readable, referencing the crd_dir variable and the
subprocess invocation that sets cwd.
alfieprojectsdev
added a commit
that referenced
this pull request
Jun 22, 2026
- _is_rinex_obs: accept Bernese .RXO files. Campaign RAW/ dirs hold .RXO RINEX Observation copies; the validator previously parsed zero stations from a real campaign and passed vacuously, defeating the pre-BPE check. - ATX coverage: compare the antenna model token instead of a substring, matching _ant_types_match semantics. A substring test let "LEIAR10" spuriously match an ATX entry "LEIAR10R", masking an uncalibrated antenna. - orchestrator.run_velocity_pipeline: validate crd_dir up front instead of deferring to subprocess cwd (CodeRabbit PR #37). - tests: .RXO detection, ATX substring false-positive, crd_dir/script guards (55 -> 59).
Owner
Author
|
Superseded by main. BRN-005 + BRN-006 re-applied as parallel commits (e1f2de2, c0b23ca, 8ea89ba); main now strictly ahead via c002a88 (.RXO recognition, ATX token-match, crd_dir guard, +tests — the CodeRabbit fixes plus more) and 7f79174/ANA-001. Merge would conflict and regress those. No unique content on branch. |
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
1 participant
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.
Summary
6bf92fc):rinex_header_validator.py— reads RINEX OBS headers from campaignRAW/, cross-checksREC TYPEandANT TYPEagainst.STATYPE 002 and the staged ATX file; raisesValidationErrorwith the full mismatch list before any BPE run. Wired intoLinuxBPEBackend.run()as a pre-flight gate. Directly addresses the staff-identified Add comprehensive code review for VADASE RT and ingestion pipeline #1 Bernese pain point: RXOBV3 (PID 221/222) silently dropping stations with mismatched equipment metadata.9c812ee):RUNX_v2.pynow accepts--reference-station / -r; all threeinput()gates and the trailingtime.sleep(3)are skipped in headless mode while preserving the existing interactive staff workflow when the flag is omitted.BerneseOrchestrator.run_velocity_pipeline()added as the post-BPE hook.Key design decisions
"LEIAR20 NONE"(RINEX) vs"LEIAR20"(STA) is treated as a match by comparing first whitespace-delimited tokens. Full-string mismatch is still caught.missing_from_rawis a warning, not an error: a station in STA with no RINEX in RAW/ is expected (data may be staged later). Only the reverse (missing_from_sta) is a hard failure — those stations will be silently dropped by RXOBV3.ATM/, so campaigns without a calibration file don't fail pre-flight.outlier_input-site.pydeliberately left interactive: that human review gate is the intended scope of the velocity-reviewer web UI, per ticket spec.Test coverage
ValidationErrormessage formatting,LinuxBPEBackendintegrationuv run pytest services/bernese-workflow/tests/ -v)Test plan
uv run pytest services/bernese-workflow/tests/ -v— 55 tests greenREC TYPEin a RINEX header; confirmValidationErrorraised with station + field in message before any Perl subprocess spawnspython "analysis/02 Time Series/RUNX_v2.py" --reference-station BOSTfrom a CRD directory — confirm no prompts appearpython "analysis/02 Time Series/RUNX_v2.py"(no flag) — confirm existing interactive prompts still appear🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests