feat(bernese): BRN-002+003 BPEBackend protocol + LinuxBPEBackend + PHIVOL_REL PCF template - #36
Merged
Merged
Conversation
… template BRN-002: BPEBackend Protocol (prepare_campaign/run/collect_outputs), LinuxBPEBackend invoking perl rnx2snx_pcs.pl with 2-hour timeout, BPEResult dataclass, _parse_bpe_output for quality gates (PID 221/443/513/514 — RXOBV3 station count, ambiguity fixing rate, HELMCHK reference-station motion, COMPARF daily repeatability), WindowsBPEBackend stub. BRN-003: Replace 16-line placeholder PCF with full PHIVOL_REL.PCF- derived Jinja2 template (63 PIDs, 7 server variable overrides via PCFContext: v_crdinf, v_rnxdir, v_b, v_refinf, v_sampl, v_satsys, v_hoifil). Fixed-width PCF format preserved; Jinja2 vars only in VARIABLE section DEFAULT column. 14 tests: 11 backend unit tests, 3 orchestrator tests including round-trip render of real PHIVOL_REL template.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a typed BPE backend API and Linux implementation, refactors the orchestrator to use the backend, introduces a typed PCF rendering context, replaces the PCF template with Jinja-parameterized content, and adds unit tests for parsing, backend prep/collection, and orchestrator wiring. ChangesBPE Backend, Orchestrator Integration & Template Context
Sequence DiagramsequenceDiagram
actor User
participant Orchestrator
participant Backend
participant PerlScript as "Perl Script"
participant FileSystem
User->>Orchestrator: run_bpe(campaign, year, session)
Orchestrator->>Backend: prepare_campaign(campaign, year, session)
Backend->>FileSystem: create campaign dirs (ATM, BPE, OBS, ...)
Orchestrator->>Backend: run(campaign, year, session)
Backend->>PerlScript: subprocess.run rnx2snx_pcs.pl (env overrides, timeout)
PerlScript->>FileSystem: write OUT/*.SNX and OUT/*.NQ0
PerlScript-->>Backend: stdout/stderr (log text)
Backend->>Backend: _parse_bpe_output(log text)
Backend->>Backend: collect_outputs -> glob OUT files
Backend-->>Orchestrator: return BPEResult(success, metrics, files, raw_log)
Orchestrator-->>User: forward BPEResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Review rate limit: 8/10 reviews remaining, refill in 10 minutes and 49 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
services/bernese-workflow/src/bernese_workflow/backends.py (2)
112-113: 💤 Low valueMove
import osto module level.The import statement inside the method works but is unconventional. Module-level imports improve readability and allow static analyzers to track dependencies.
♻️ Suggested fix
Add at the top of the file with other imports:
import osThen remove line 112.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/bernese-workflow/src/bernese_workflow/backends.py` around lines 112 - 113, Move the in-function import to module scope: remove the inline "import os" inside the function where env = {**os.environ, **env_overrides} is built, and add a single "import os" alongside the file's other top-level imports so os is available throughout the module; this affects the function that constructs the env dict (where env_overrides is merged) and ensures static analyzers and readability are improved.
25-25: 💤 Low valueMinor: EN DASH character in comment.
The static analyzer flagged an EN DASH (
–) instead of a standard hyphen (-) in the comment. This is cosmetic but could cause issues if the comment is ever parsed.♻️ Suggested fix
- ambiguity_fixing_rate: float | None # from PID 443 AMBXTR, 0.0–1.0 + ambiguity_fixing_rate: float | None # from PID 443 AMBXTR, 0.0-1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/bernese-workflow/src/bernese_workflow/backends.py` at line 25, The comment on the type annotation for ambiguity_fixing_rate contains an EN DASH (–); replace it with a standard ASCII hyphen (-) so the comment reads "0.0-1.0" (update the comment attached to ambiguity_fixing_rate: float | None to use "-" instead of "–").services/bernese-workflow/src/bernese_workflow/pcf_context.py (1)
18-27: 💤 Low valueConsider using
dataclasses.asdict()for automatic field mapping.The manual
to_dict()implementation works but requires updating when fields are added.dataclasses.asdict()would automatically include all fields.♻️ Suggested simplification
+from dataclasses import asdict, dataclass -from dataclasses import dataclass `@dataclass` class PCFContext: ... def to_dict(self) -> dict[str, str]: - return { - "v_crdinf": self.v_crdinf, - "v_rnxdir": self.v_rnxdir, - "v_b": self.v_b, - "v_refinf": self.v_refinf, - "v_sampl": self.v_sampl, - "v_satsys": self.v_satsys, - "v_hoifil": self.v_hoifil, - } + return asdict(self)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/bernese-workflow/src/bernese_workflow/pcf_context.py` around lines 18 - 27, The current to_dict method manually maps fields and will break when fields are added; update pcf_context.PcfContext.to_dict to return dataclasses.asdict(self) instead: import asdict from dataclasses and replace the manual dict with asdict(self); if you must preserve return type dict[str, str], map/convert values to strings after asdict (e.g., {k: str(v) for k,v in asdict(self).items()}) to keep the signature consistent.services/bernese-workflow/src/bernese_workflow/orchestrator.py (1)
27-34: ⚖️ Poor tradeoffHardcoded
GPSUSERandGPSDATAsubdirectory paths may limit flexibility.The default
LinuxBPEBackendassumesGPSUSERandGPSDATAexist as subdirectories ofbernese_path. This structure may not match all Bernese installations where these directories can be located elsewhere.Consider allowing these paths to be configurable, or document the expected directory structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py` around lines 27 - 34, The constructor currently hardcodes GPSUSER and GPSDATA under bernese_path when instantiating LinuxBPEBackend (see self._backend and LinuxBPEBackend), which reduces flexibility; modify the orchestrator initializer to accept optional user_dir and campaign_dir parameters (or allow passing a fully-configured backend) and pass those values into LinuxBPEBackend instead of Path(bernese_path) / "GPSUSER" and Path(bernese_path) / "GPSDATA", or fall back to the existing defaults; update the BPEBackend/LinuxBPEBackend construction to use these configurable values and document the new parameters in the docstring.services/bernese-workflow/tests/test_backends.py (1)
76-77: ⚡ Quick winConsider importing
_SUBDIRSfrom the module to avoid duplication.The subdirectory list is duplicated from
backends.py. If subdirectories are added or removed, this test would need manual updating.♻️ Suggested fix
from bernese_workflow.backends import ( LinuxBPEBackend, WindowsBPEBackend, _parse_bpe_output, + _SUBDIRS, ) # ... def test_linux_backend_prepare_creates_subdirs(tmp_path): # ... campaign_path = tmp_path / "GPSDATA" / "TESTCAMP" - for subdir in ("ATM", "BPE", "GRD", "OBS", "ORB", "ORX", "OUT", "RAW", "SOL", "STA"): + for subdir in _SUBDIRS: assert (campaign_path / subdir).is_dir(), f"Missing subdir: {subdir}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/bernese-workflow/tests/test_backends.py` around lines 76 - 77, Replace the hard-coded tuple of subdirectories in the test with the authoritative list from the backend module: import _SUBDIRS from backends (the symbol is _SUBDIRS) and iterate over that instead of the literal ("ATM", "BPE", ...). Update the assertion loop in tests/test_backends.py to use the imported _SUBDIRS so future changes in backends.py are picked up automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/bernese-workflow/src/bernese_workflow/backends.py`:
- Around line 112-113: Move the in-function import to module scope: remove the
inline "import os" inside the function where env = {**os.environ,
**env_overrides} is built, and add a single "import os" alongside the file's
other top-level imports so os is available throughout the module; this affects
the function that constructs the env dict (where env_overrides is merged) and
ensures static analyzers and readability are improved.
- Line 25: The comment on the type annotation for ambiguity_fixing_rate contains
an EN DASH (–); replace it with a standard ASCII hyphen (-) so the comment reads
"0.0-1.0" (update the comment attached to ambiguity_fixing_rate: float | None to
use "-" instead of "–").
In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py`:
- Around line 27-34: The constructor currently hardcodes GPSUSER and GPSDATA
under bernese_path when instantiating LinuxBPEBackend (see self._backend and
LinuxBPEBackend), which reduces flexibility; modify the orchestrator initializer
to accept optional user_dir and campaign_dir parameters (or allow passing a
fully-configured backend) and pass those values into LinuxBPEBackend instead of
Path(bernese_path) / "GPSUSER" and Path(bernese_path) / "GPSDATA", or fall back
to the existing defaults; update the BPEBackend/LinuxBPEBackend construction to
use these configurable values and document the new parameters in the docstring.
In `@services/bernese-workflow/src/bernese_workflow/pcf_context.py`:
- Around line 18-27: The current to_dict method manually maps fields and will
break when fields are added; update pcf_context.PcfContext.to_dict to return
dataclasses.asdict(self) instead: import asdict from dataclasses and replace the
manual dict with asdict(self); if you must preserve return type dict[str, str],
map/convert values to strings after asdict (e.g., {k: str(v) for k,v in
asdict(self).items()}) to keep the signature consistent.
In `@services/bernese-workflow/tests/test_backends.py`:
- Around line 76-77: Replace the hard-coded tuple of subdirectories in the test
with the authoritative list from the backend module: import _SUBDIRS from
backends (the symbol is _SUBDIRS) and iterate over that instead of the literal
("ATM", "BPE", ...). Update the assertion loop in tests/test_backends.py to use
the imported _SUBDIRS so future changes in backends.py are picked up
automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b934c904-9f45-4d09-9c54-fd013e9e30eb
📒 Files selected for processing (6)
services/bernese-workflow/src/bernese_workflow/backends.pyservices/bernese-workflow/src/bernese_workflow/orchestrator.pyservices/bernese-workflow/src/bernese_workflow/pcf_context.pyservices/bernese-workflow/templates/basic_processing.pcf.j2services/bernese-workflow/tests/test_backends.pyservices/bernese-workflow/tests/test_orchestrator.py
- Move 'import os' to module level in backends.py - Replace EN dash with ASCII hyphen in ambiguity_fixing_rate comment - Use dataclasses.asdict() in PCFContext.to_dict() — auto-includes new fields without manual updates - Add user_dir/campaign_dir params to BerneseOrchestrator.__init__ so Bernese installs where GPSUSER/GPSDATA are not under bernese_root work - Import _SUBDIRS from backends in test_backends — avoids duplication
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/bernese-workflow/src/bernese_workflow/backends.py`:
- Around line 144-153: collect_outputs currently ignores the year and session
params and returns the first *.SNX/*.NQ0 it finds, which can attach stale
artifacts; update collect_outputs (and its use of self.campaign_dir /
campaign_name / "OUT") to filter candidate files by year and session (e.g.,
require filenames to contain the year and session tokens or use a glob pattern
that includes them) and then: if no match return empty dict, if exactly one
match use it, and if multiple matches raise a clear error
(ValueError/RuntimeError) asking the caller to disambiguate instead of silently
picking the first file.
- Around line 105-123: The subprocess is started with env built from
env_overrides but omits the required Bernese path variables provided to the
constructor (bernese_root, user_dir, campaign_dir), so export those into the
child environment before calling subprocess.run; update env_overrides (or merge
into env) to include the keys expected by Bernese ($X, $U, $P or whatever env
names your code expects) using the constructor fields bernese_root, user_dir,
and campaign_dir so that the subprocess.run call receives them.
In `@services/bernese-workflow/src/bernese_workflow/orchestrator.py`:
- Around line 25-28: The Jinja environment currently allows missing variables to
render silently; update the template environment creation (the template_env
instantiation via jinja2.Environment) to use strict undefined handling so
missing required PCFContext variables (e.g. v_crdinf, v_rnxdir) raise errors.
Specifically, pass undefined=jinja2.StrictUndefined (or import StrictUndefined)
into jinja2.Environment where template_env is created to cause rendering to fail
fast when required fields are absent.
🪄 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: 93fbaf15-35ed-42b8-ad38-a7895ac06032
📒 Files selected for processing (4)
services/bernese-workflow/src/bernese_workflow/backends.pyservices/bernese-workflow/src/bernese_workflow/orchestrator.pyservices/bernese-workflow/src/bernese_workflow/pcf_context.pyservices/bernese-workflow/tests/test_backends.py
✅ Files skipped from review due to trivial changes (1)
- services/bernese-workflow/tests/test_backends.py
🚧 Files skipped from review as they are similar to previous changes (1)
- services/bernese-workflow/src/bernese_workflow/pcf_context.py
- Export X/U/P env vars (bernese_root/user_dir/campaign_dir) into the subprocess env so Perl scripts can locate Bernese directories - collect_outputs: raise RuntimeError on ambiguous SNX/NQ0 matches; silently returning the first file attached stale artifacts to BPEResult - Use jinja2.StrictUndefined so missing required PCFContext vars (v_crdinf, v_rnxdir) raise UndefinedError at render time, not silently produce empty strings in the PCF - 3 new collect_outputs tests, 1 new StrictUndefined test (18 total)
4 tasks
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
BPEBackendProtocol +LinuxBPEBackend+BPEResultdataclass — real Perl BPE invocation with quality gate parsingPHIVOL_REL.PCF-derived Jinja2 template (63 PIDs, 7 parameterized server variables)PCFContextdataclass for typed template renderingBRN-002 detail
backends.pyintroduces:BPEBackend—typing.Protocolwithprepare_campaign(),run(),collect_outputs()BPEResult— dataclass holding success flag + all four quality gate metricsLinuxBPEBackend— invokesperl $U/SCRIPT/rnx2snx_pcs.pl YEAR SESSIONvia subprocess with a 2-hour timeout; setsPCF_FILE,CPU_FILE,BPE_CAMPAIGN,YEAR,SESSIONenv vars; calls_parse_bpe_output()on the combined stdout+stderr_parse_bpe_output()— parses the four BPE quality gates:0.823and82.3 %formats)WindowsBPEBackend— stub satisfying the Protocol; raisesNotImplementedErrorBerneseOrchestratorupdated: acceptsbackend: BPEBackend | None; defaults toLinuxBPEBackend;run_bpe()now takesyear: int, session: strBRN-003 detail
basic_processing.pcf.j2is now the full PHIVOL_REL.PCF workflow (63 PIDs across 7 stages). Jinja2 variables appear only in the VARIABLE section DEFAULT column — the fixed-width PID/SCRIPT/OPT_DIR columns are unchanged. Seven overrides parameterized viaPCFContext:v_crdinfv_rnxdirv_bIGSv_refinfIGS14v_sampl180v_satsysGPSv_hoifilHOI$YSS+0Test plan
uv run pytest services/bernese-workflow/tests/ -v→ 14 passeduv run ruff check services/bernese-workflow/→ no issuestest_generate_pcf_phivol_template— end-to-end render of real template; assertsPIVSMIND,000 FTP_DWLD,443 AMBXTR,514 HELMCHKall present in outputDepends on: BRN-001 (R740 install) for real execution — backend is tested via mocks only.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
Tests