pipe: carry rotor scan resolution through the task payload - #1027
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a silent correctness issue in piped 1D rotor scans by ensuring the intended rotor scan resolution is preserved across the pipe boundary (staging → worker execution), rather than implicitly falling back to whatever resolution is configured on the worker node.
Changes:
- Capture
scan_resduring task staging inbuild_rotor_scan_1d_tasksand store it in eachrotor_scan_1dtask payload. - Forward
scan_reson the worker side via the existing troubleshooting override channelargs['trsh']['scan_res']. - Add targeted unit tests covering both staging-time capture and worker-time forwarding / fallback behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| arc/scripts/pipe_worker.py | Extracts scan_res from rotor_scan_1d payload and forwards it via args['trsh']['scan_res'] when present. |
| arc/scripts/pipe_worker_test.py | Adds unit tests for _get_family_extra_kwargs to ensure scan_res forwarding and correct omission when absent. |
| arc/job/pipe/pipe_run.py | Extends build_rotor_scan_1d_tasks to accept optional scan_res and include it in the task payload. |
| arc/job/pipe/pipe_run_test.py | Adds tests to confirm scan_res is carried into payload when provided and omitted when None. |
| arc/job/pipe/pipe_planner.py | Passes the staging-side rotor scan resolution into build_rotor_scan_1d_tasks so workers don’t revert to their local defaults. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1027 +/- ##
==========================================
- Coverage 65.52% 65.51% -0.01%
==========================================
Files 120 120
Lines 40468 40472 +4
Branches 10430 10431 +1
==========================================
+ Hits 26515 26516 +1
- Misses 10948 10950 +2
- Partials 3005 3006 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2fabe30 to
0398bc8
Compare
The rotor_scan_1d TaskSpec held no scan_res, so a piped ASE/MLIP scan ran the worker node's rotor_scan_resolution setting instead of ARC's intended one. Capture it at staging time and forward it in the worker as trsh scan_res.
Ophir-Weisz
left a comment
Ophir-Weisz
left a comment
There was a problem hiding this comment.
Traced the worker path — job_factory gets args from nowhere else, so the trsh channel
is clean and doesn't clobber anything.
One thing before I approve: line 313 forwards the settings default rather than
Scheduler.rotor_scan_resolution from #1019, so a user-set resolution looks like it'd
reach ESS scans but not piped rotors. Details inline, plus a scope question on the
18-point guard.
| self.sched.species_dict[label], label, rotor_indices, | ||
| self._level_dict(level), adapter, self._memory_mb), | ||
| self._level_dict(level), adapter, self._memory_mb, | ||
| scan_res=rotor_scan_resolution), |
There was a problem hiding this comment.
blocking: This forwards the settings default, not the run-level value. #1019 (now merged)
stores the input key on Scheduler.rotor_scan_resolution and injects it via
set_scan_resolution for job_type == 'scan' — and its description says the pipe path is
untouched, so this PR is the one meant to close that.
As written, a user who sets rotor_scan_resolution: 4.0 in their input gets 4.0 on ESS
scans and 8.0 on piped rotors: the same silent rebinding this PR is fixing, just moved
from the worker's settings to the host's.
self.sched is already in this call two lines up (line 311), so something like
self.sched.rotor_scan_resolution or rotor_scan_resolution would prefer the run's value
and keep the settings default as the fallback. Am I reading the precedence right?
| logger = get_logger() | ||
|
|
||
| pipe_settings = settings['pipe_settings'] | ||
| rotor_scan_resolution = settings['rotor_scan_resolution'] |
There was a problem hiding this comment.
question: #1019 refuses a resolution coarser than 20 deg with InputError at parse time,
because below 18 points the Fourier fit never runs. That guard sits on the input key
only — a machine-wide settings override (which is the incident #1019's description
opens with) reaches this line unguarded, and check_argument_consistency only tests
divmod(360, scan_res)[1], which 60.0 passes.
Not a regression, since the worker read the same settings before. But this PR is framed
as closing the same silent-wrong-answer class, so: should the 18-point guard apply here
too, or is settings.py considered trusted in a way the input file isn't?
Motivation
A piped 1D rotor scan (
rotor_scan_1d, the path MLIP/ASE rotors take) ran at whateverrotor_scan_resolutionthe worker node's settings happened to hold — not the resolution the runintended. The
TaskSpeccarried noscan_res, so the value simply rebound on the compute node.This is the same silent-wrong-answer class as #1019: below 18 points per rotor RMG-Py's Fourier
fitter never runs and
get_potential()reads an uninitialised C double. Nothing crashes; thetorsional contribution is just wrong. #1019 makes resolution a property of the run at the input
layer; this closes the remaining hole, where the value was lost crossing the pipe boundary.
What changed
Capture the resolution at staging time, on the machine that knows what the run asked for, and
forward it in the worker:
pipe_planner.py/pipe_run.py—build_rotor_scan_1d_tasksrecordsscan_resinto therotor_scan_1dtask payload.pipe_worker.py— forwards it asargs['trsh']['scan_res'], the existing per-job overridechannel (the same one Add a rotor_scan_resolution input key #1019 uses), so no new mechanism is introduced.
Absent a
scan_resin the payload, behaviour is unchanged: the worker's settings default stillapplies, exactly as before.
Tests
New coverage in
pipe_run_test.pyandpipe_worker_test.pypinning that the resolution iscaptured at staging and forwarded at execution.
arc/job/pipe/: 157 passed.arc/job/adapters/ase_test.py arc/job/adapters/common_test.py arc/scheduler_test.py: 86 passed.Run under
arc_envwithHOMEpointed at an empty dir (ARC's suite reads~/.arc/settings.py),-n0 -o addopts="".An end-to-end check against the benchmark: an ethane pipe scan matched the reference potential to
0.0279 kJ/mol.
Relationship to other PRs
Independent of #1019 and mergeable on its own — they close the same gap at two different layers
(#1019 at the input/scheduler boundary for ESS
scanjobs, this at the pipe boundary for pipedrotor tasks). Rebased onto
mainnow that #1018 (the one-process ASE relaxed scan this builds on)has merged.