Skip to content

pipe: carry rotor scan resolution through the task payload - #1027

Open
alongd wants to merge 1 commit into
mainfrom
i002-pipe-rotor
Open

pipe: carry rotor scan resolution through the task payload#1027
alongd wants to merge 1 commit into
mainfrom
i002-pipe-rotor

Conversation

@alongd

@alongd alongd commented Aug 26, 2026

Copy link
Copy Markdown
Member

Motivation

A piped 1D rotor scan (rotor_scan_1d, the path MLIP/ASE rotors take) ran at whatever
rotor_scan_resolution the worker node's settings happened to hold
— not the resolution the run
intended. The TaskSpec carried no scan_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; the
torsional 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.pybuild_rotor_scan_1d_tasks records scan_res into the
    rotor_scan_1d task payload.
  • pipe_worker.py — forwards it as args['trsh']['scan_res'], the existing per-job override
    channel (the same one Add a rotor_scan_resolution input key #1019 uses), so no new mechanism is introduced.

Absent a scan_res in the payload, behaviour is unchanged: the worker's settings default still
applies, exactly as before.

Tests

New coverage in pipe_run_test.py and pipe_worker_test.py pinning that the resolution is
captured 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_env with HOME pointed 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 scan jobs, this at the pipe boundary for piped
rotor tasks). Rebased onto main now that #1018 (the one-process ASE relaxed scan this builds on)
has merged.

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_res during task staging in build_rotor_scan_1d_tasks and store it in each rotor_scan_1d task payload.
  • Forward scan_res on the worker side via the existing troubleshooting override channel args['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

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.83%. Comparing base (e609ca0) to head (2fabe30).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1027      +/-   ##
==========================================
+ Coverage   64.77%   64.83%   +0.05%     
==========================================
  Files         119      119              
  Lines       40011    40015       +4     
  Branches    10341    10342       +1     
==========================================
+ Hits        25918    25944      +26     
+ Misses      11110    11088      -22     
  Partials     2983     2983              
Flag Coverage Δ
functionaltests 64.83% <ø> (+0.05%) ⬆️
unittests 64.83% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants