Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #985 +/- ##
=======================================
Coverage 66.26% 66.27%
=======================================
Files 122 122
Lines 41824 41874 +50
Branches 10751 10754 +3
=======================================
+ Hits 27716 27752 +36
- Misses 11062 11072 +10
- Partials 3046 3050 +4
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:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes the previously-latent “queue execution” path for ASEAdapter so ASE-backed calculators (e.g., UMA/fairchem MLIPs) can run as real cluster jobs, including rotor/directed-scan workloads that ARC dispatches via the scheduler.
Changes:
- Write
submit+input.ymlduring adapter construction for non-incore ASE jobs soupload_files()succeeds. - Replace ASE’s bespoke queue submission logic with the shared
legacy_queue_execution()flow. - Correct ASE internal constraint indexing by translating ARC’s 1-indexed constraints to ASE
FixInternals’ 0-indexed expectations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| arc/job/adapters/ase_adapter.py | Writes queue job files in set_files(), adds directed-scan constraints translation, and delegates queue submission to legacy_queue_execution(). |
| arc/job/adapters/ase_test.py | Adds unit coverage asserting queue-job construction writes/uploadables exist on disk (and that incore does not). |
| arc/job/adapters/scripts/ase_script.py | Converts constraint atom indices from ARC’s 1-indexing to ASE’s 0-indexing before applying FixInternals. |
Suppressed comments (1)
arc/job/adapters/ase_adapter.py:415
- This method always writes the submit script to submit.sh. On Slurm, ARC submits submit.sl (see settings['submit_filenames']), so Slurm jobs will still error unless the script is written under the Slurm filename.
with open(os.path.join(self.local_path, 'submit.sh'), 'w') as f:
f.write(content)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Added a third commit ( |
5848337 to
65e4d98
Compare
65e4d98 to
76b7b19
Compare
76b7b19 to
3489549
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Suppressed comments (6)
arc/job/adapters/ase_adapter.py:284
- Slurm jobs will still fail at submission:
write_submit_script()writessubmit.sh, and this entry uploads/registers that same name, butlegacy_queue_execution()invokes the sharedsubmit_filenames['Slurm'](submit.sl; seearc/settings/settings.py:173-176). Thussbatch submit.slcannot find the file. Use the shared scheduler-specific filename consistently when writing and registering the script.
self.files_to_upload.append(self.get_file_property_dictionary(file_name='submit.sh'))
arc/job/adapters/ase_adapter.py:354
max_job_timeis not propagated into these templates:format_kwargshas not_max, and the PBS/Slurm templates have no walltime directive. Unlike the sharedJobAdapter.write_submit_script()path (arc/job/adapter.py:287-315), an ASE queue job will use the scheduler default limit and can be killed before ARC's requested time. Add the scheduler-specific walltime directive and pass the formatted value.
format_kwargs = {'name': self.job_server_name, 'cpus': self.cpu_cores, 'memory': memory,
'remote_path': self.remote_path, 'env_setup': config['env_setup'], 'command': command}
arc/job/adapters/ase_adapter.py:376
queue_jobis false for every non-incore scheduler other than PBS/Slurm, so OGE/HTCondor (and a no-queuelocalserver) take the bare-script branch but are still submitted throughlegacy_queue_execution(). HTCondor then looks forsubmit.sub, while onlysubmit.shwas written; other schedulers also lack their directives. Raise for unsupported schedulers or add templates instead of silently submitting an invalid script.
queue_job = self.execution_type != 'incore' and cluster_soft in ('pbs', 'slurm')
arc/job/adapters/scripts/ase_script.py:127
- The new index conversion is not covered by the ASE tests: there is no test for
apply_constraints()or fordetermine_constraints()generating a directed-scan constraint. A regression here would silently apply a constraint to the wrong atoms, so add a test that writes a 1-indexed four-atom dihedral constraint and verifies theFixInternalsindices are 0-indexed.
indices = [index - 1 for index in constraint[0]]
arc/settings/submit.py:128
- The PR description says the diff is confined to three ASE adapter files, but this change also adds
arc/settings/submit.pyand modifiesarc/imports.py. Please update the description or explain these additional shared submission/configuration changes, since they materially expand the review scope.
# Submission scripts for queue-executed ASE (e.g. UMA/fairchem MLIP) jobs, keyed by cluster
arc/settings/submit.py:146
- ARC invokes
sbatchafter changing into the submission directory, but this script changes intoremote_pathas if the batch job starts at the remote home. When the server has nopath,remote_pathis relative, so this can resolve to a nestedruns/.../runs/...directory; for a local queue it also points away fromlocal_path. Use the scheduler's submission directory for the job directory instead.
cd "{remote_path}"
JOB_DIR="$(pwd)" # absolute path (the path above is relative to the remote home, where the job starts)
cb346da to
6e97a32
Compare
…/fairchem) jobs runnable on a cluster
…/fairchem) jobs runnable on a cluster
…/fairchem) jobs runnable on a cluster
…/fairchem) jobs runnable on a cluster
…/fairchem) jobs runnable on a cluster
35e6091 to
6c8424e
Compare
454352d to
d7d3a1f
Compare
ASEAdapter.write_submit_script() emitted a bare two-line bash script: no scheduler directives, no queue, no environment activation, and the ARC host's conda python path, which does not exist on the server. It now composes a PBS or Slurm script from args['block'] (queue, env_setup, gpu_resource, python), pins the thread pools to the granted core count so torch cannot oversubscribe a shared node, requests the job's walltime so a long scan is not killed at the queue default, and stamps initial_time/final_time so ARC can report a run time. The script body lives in a server-independent ase_submit template in arc/settings/submit.py (keyed by cluster software, mirroring pipe_submit and wired through arc.imports with the same local-override hook), so it can be customized per cluster like every other submit script; the adapter only fills in placeholders. The script is written under the scheduler's submit filename (submit.sl for Slurm), which is the name submit_job() invokes, and cd's into the submission directory (the local path for a 'local' server). The resolved queue is recorded in attempted_queues, as JobAdapter does, so a failed submission moves on to the next queue instead of retrying the same one. Incore jobs keep the bare script. A queue-executed ASE job also never wrote its submit script or input.yml. set_files() only listed them for upload, but JobAdapter.execute() uploads before it calls execute_queue(), so the upload died with "InputError: Cannot upload a non-existing file". Write them in set_files(), where Gaussian, Orca and xTB write theirs; the incore path still writes its input in execute_incore(). execute_queue() then never submitted anything either: it guarded on self.server_adapter, an attribute nothing sets. Use legacy_queue_execution(), as every other adapter does, which also records the job id and status. Directed scans were also not constrained: Scheduler.run_job() always passes constraints=None and hands the adapter torsions + dihedrals instead, so every point of a brute_force_opt scan optimized freely and relaxed to the same minimum. ASEAdapter.determine_constraints() derives the constraint, and apply_constraints() converts ARC's 1-indexed atom indices to ASE's 0-indexed FixInternals. The shared Scheduler/Gaussian side of that defect is left alone here; a Gaussian directed_scan job needs its own fix.
d7d3a1f to
eac1707
Compare
|
Rebased onto current @calvinp0 — both of your points:
Copilot's four:
CodeQL's two: implicit string concatenation in the submit list, and the bare Tests: One note for whoever reviews: a separate defect in this same area is up as #1054 ( |
…/fairchem) jobs runnable on a cluster
| config = self.determine_submit_config() | ||
| cluster_soft = servers.get(self.server, dict()).get('cluster_soft', '').lower() \ | ||
| if self.server is not None else '' | ||
| queue_job = self.execution_type != 'incore' and cluster_soft in ('pbs', 'slurm') |
There was a problem hiding this comment.
should we also deal with htcondor and oge?
|
|
||
| def test_set_files_does_not_write_for_an_incore_job(self): | ||
| """Test that an incore job writes no submit script (it writes its input when it executes)""" | ||
| self.assertFalse(os.path.isfile(os.path.join(self.job_1.local_path, 'submit.sh'))) |
There was a problem hiding this comment.
the assertion can't fail; setUpClass reassigns local_path after construction, so it checks a directory set_files() never wrote to.
| """ | ||
| cluster_soft = servers.get(self.server, dict()).get('cluster_soft', '') if self.server is not None else '' | ||
| if self.execution_type != 'incore' and cluster_soft.lower() in ('pbs', 'slurm'): | ||
| return submit_filenames[cluster_soft] |
There was a problem hiding this comment.
we do a lower case guard but then the lookup isn't lowercase
`determine_submit_filename()` guarded on `cluster_soft.lower() in ('pbs', 'slurm')`
but then looked the name up as `submit_filenames[cluster_soft]`, verbatim. Since
that dict is keyed by the exact settings spelling ('PBS', 'Slurm'), a server whose
`cluster_soft` is spelled any other way passed the guard and raised KeyError while
the job was still being constructed.
The lookup is the correct half: `local.submit_job()`/`ssh.submit_job()` resolve the
file to submit the same verbatim way, so lowercasing here would name a file the
submission never invokes. Tightened the guard to the lookup's own precondition via
a `QUEUE_CLUSTER_SOFT` constant, used by `write_submit_script()` too so both agree,
and kept the lower-cased key only where it belongs - `ase_submit`, ARC's own dict.
A queue job on a scheduler with no `ase_submit` template (HTCondor, OGE) now says
so rather than silently writing a bare script with no scheduler directives.
`test_set_files_does_not_write_for_an_incore_job` could not fail: `setUpClass` repoints `local_path` at a flat scratch directory after construction, so the assertion checked a directory `set_files()` had never written to. It now asserts against the path the adapter resolved for itself, which `set_files()` actually used. Added regression tests for the rest of the queue path, each of which fails against the defect it covers: the submit script is written under the filename `submit_job()` invokes (Slurm's `submit.sl`, not `submit.sh`); an unknown `cluster_soft` spelling falls back instead of raising; a job on the `local` server cd's into its local path rather than a remote path that is not there; the resolved queue reaches `attempted_queues`, without which `trsh_job_queue()` retries the same queue forever; and `max_job_time` reaches the script instead of the queue default.
|
Worked every open finding on this PR. Pushed two commits (fast-forward, no rebase, no force-push): Short version: one of the nine was still a live defect (finding 1). Five had already been fixed on the branch but carried no test, so they now have one. Two were answered. One was already resolved. 1.
|
| module | before | after |
|---|---|---|
arc/job/adapters/ase_test.py |
21 | 27 (+7 subtests) |
arc/level_test.py |
17 | 17 |
arc/job/adapter_test.py |
42 | 42 |
arc/imports_test.py |
11 | 11 |
| total | 91 | 97 |
Wider sweep over arc/job/ + arc/settings/ + arc/level_test.py + arc/imports_test.py: 1515 passed, 5 skipped, 2 failed. Both failures are pyscf_test.py::test_run_freq_*, from PySCF not being installed in this environment; I re-ran them on the unmodified tip and they fail identically there, so they are not from this work.
One thing left for you
The branch is BEHIND main (mergeStateStatus: BEHIND) and needs a rebase before it can merge. I deliberately did not do it — rebasing and force-pushing a branch that is under review is yours to call, not mine.
What
Make queue-executed
ASEAdapterjobs actually run on a remote cluster. The ASE adapter had only ever been exercised incore; its queue path was latent and broke on the first real submission. This lets ASE-backed calculators (e.g. UMA / fairchem MLIPs) run hindered-rotor and other scans as ordinary PBS/Slurm jobs.Why
Running a real ARC job that delegates its hindered-rotor scans to an ASE calculator on a cluster queue crashed the moment the first scan job was dispatched:
Two independent defects in the queue path, both masked because ASE had only ever run incore:
JobAdapter.execute()callsupload_files()beforeexecute_queue(), and_initialize_adapter()callsset_files()while the job is constructed. The Gaussian/Orca/xTB adapters writesubmit.sh/ input insideset_files();ASEAdapter.set_files()only appended their names tofiles_to_uploadand never wrote them, so the calcs dir was empty whenssh.upload_file()ran itsos.path.isfilepre-flight.ASEAdapter.execute_queue()guarded onself.server_adapter, an attribute nothing in ARC sets (hasattr→False), so even with the files present the job would never submit — the write fix alone just moves the crash one line down to anAttributeError.Changes (confined to the ASE adapter)
set_files()now writessubmit.shandinput.ymlfor non-incore jobs, mirroring the other adapters. Incore is unchanged — it still writes its input inexecute_incore().execute_queue()now delegates to the sharedlegacy_queue_execution()that every other adapter uses (which also recordsjob_status/job_id), replacing the bespoke never-reached block.write_submit_script()'s non-queue branch falls back tolocal_pathwhenremote_pathisNone(a server-less job), fixing a latent crash surfaced by writing at construction time.test_set_files_writes_the_files_of_a_queue_job(red against the pre-fix adapter).Verification
pytest arc/job/adapters/ase_test.py -q -n0→ 10 passed (the file requires serial-n0, a pre-existing xdistsetUpClassrace unrelated to this change).execute()→execute_incore()→ase_script.py→parse_results()reproduces the prior geometry/energy to 5 decimals.submit.sh+input.ymland submitted to the queue, where the pre-fix run crashed.The diff is limited to
arc/job/adapters/ase_adapter.py,arc/job/adapters/ase_test.py, and a 1-indexed→0-indexed constraint fix inarc/job/adapters/scripts/ase_script.py(ASEFixInternalsis 0-indexed; ARC constraints are 1-indexed). No other adapter's submit path is touched.