Delete ESS checkfiles on the server too, not just locally - #1000
Open
alongd wants to merge 1 commit into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
alongd
commented
Aug 19, 2026
| """ | ||
| path = servers[server].get('path', '').lower() | ||
| path = os.path.join(path, servers[server]['un']) if path else '' | ||
| return os.path.join(path, 'runs', 'ARC_Projects', project) |
Member
Author
There was a problem hiding this comment.
not alwyas running under 'runs', 'ARC_Projects'
alongd
commented
Aug 19, 2026
| command = f'chmod{recursive} {mode} {file_name}' | ||
| self._send_command_to_server(command, remote_path) | ||
|
|
||
| def delete_check_files(self, remote_path: str) -> None: |
Member
Author
There was a problem hiding this comment.
what's the difference between this and arc.common.delete_check_files()?
ARC's `keep_checks` policy was only half implemented: `delete_check_files()` walked the local `calcs` tree, and no code path ever removed the check files ARC had uploaded to a server. They accumulate silently -- measured at 82% of a fully characterised species' remote footprint, against a managed quota. `ARC.clean_check_files()` now owns both sides of the policy: with `keep_checks` false it deletes the local check files as before and then, on every non-local server in `ess_settings`, deletes `*.chk` under that project's own remote directory. `keep_checks=True` keeps them everywhere. Cleanup runs once ARC is done with the science, so a server that cannot be reached at that point is logged and skipped rather than failing the run.
alongd
force-pushed
the
remote-check-cleanup
branch
from
August 19, 2026 22:17
5246c0e to
90d4838
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1000 +/- ##
==========================================
+ Coverage 64.38% 64.47% +0.08%
==========================================
Files 119 119
Lines 39601 39627 +26
Branches 10269 10275 +6
==========================================
+ Hits 25499 25549 +50
+ Misses 11121 11089 -32
- Partials 2981 2989 +8
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:
|
calvinp0
added a commit
that referenced
this pull request
Aug 20, 2026
Every remote job opened its own paramiko Transport for upload, submission and status polling. A TS search issuing ~100 guess optimizations against one server opened ~100 connections, which is slow and trips per-user connection limits on some clusters. Add a process-global pool (arc/job/ssh_pool.py). A remote queue job leases one client for the duration of its submission and reuses it for both file upload and run; _open_or_borrow_ssh() prefers that leased client and otherwise borrows from the pool. The borrower never closes a client it does not own. The third case, a one-shot client for when the pool itself cannot lease one, is real rather than nominal. Written as `try: return pool.borrow(server)` with an `except` around it, it could never fire: borrow is a @contextmanager, so calling it merely builds the generator, and the factory runs when the caller enters the with-block -- outside the try meant to catch it. Rebuilt around contextlib.ExitStack, the fallback covers exactly the lease, i.e. entering the pool's context manager, and nothing else. In particular it does not cover the caller's own work: a job that raises inside its with-block still raises, instead of having its failure read as a broken pool and its body re-entered against a fresh client. Two pre-existing defects in the remote path that set_file_paths() builds are fixed here as well, since a remote job's files are only reachable if that path is. The server's configured 'path' was lowercased before use, which silently rewrites any path with an uppercase component -- remote file systems are case-sensitive, and /Home/Users is not /home/users. It is now used verbatim. NOTE that this changes where files land for anyone whose configured 'path' contains uppercase: their earlier runs are under the lowercased tree, and ARC will now use the path as configured. A server with no 'path' still gets a path relative to the SSH login directory, because there is no absolute path ARC can know offline -- the home directory is the server's to report, and set_file_paths() runs at job construction with no connection open. Rooting it at '~' would be worse rather than better: _send_command_to_server quotes the remote path with shlex.quote, so the remote shell would take the tilde literally, and SFTP performs no tilde expansion at all, so both would end up creating a directory actually named '~'. Relative, which the remote shell and SFTP both resolve against the login directory, stays correct. What changes is that it is no longer silent: such a server is reported once per run, naming the setting, because an adapter that has to name a path inside an input file cannot work with it. Pooled clients also need closing when ARC exits. ssh_pool.py documented that ARC.py's main() calls reset_default_pool(), but nothing did -- every caller was a test, so pooled SSHClients were left to interpreter shutdown rather than closed. Releasing them belongs to the run rather than to the command-line entry point, so ARC.execute() does it in a finally: connections are torn down on ctrl-C and on an exception as well as on a clean run, and a consumer that drives ARC in process -- a library caller, a test, a pipe worker -- releases them too, which an ARC.py-only hook could never do. ARC.py is unchanged. The borrow itself is now one function, ssh_pool.borrow_ssh_client(), rather than a method on JobAdapter. The pool's other callers are not adapters -- Scheduler.get_server_job_ids(), server troubleshooting, the ESS survey -- and each would otherwise have grown its own copy of the lease-then-fall-back dance. _open_or_borrow_ssh() keeps only what is adapter-specific, the per-execute() leased client, and delegates the rest; the shared-client branch is contextlib.nullcontext rather than a hand-rolled generator. A pooled connection is held for the whole run and sits idle between polls, so _default_factory sets a keepalive on the transport. An SSH daemon's ClientAliveInterval or a firewall's idle timeout otherwise drops it silently: the socket stays half-open, Transport.is_active() keeps reporting True, and the pool's liveness check therefore hands out a handle whose first command hangs until TCP gives up. upload_file() and download_file() were the only SSHClient methods reaching for self._sftp without @check_connections. That was harmless while every caller opened its own client and used it immediately; with a client that has been alive for hours it is not, since a dead transport surfaces as the transfer failing rather than as a reconnect. Both are decorated now. The pool is tested directly rather than only through a JobAdapter, so its own contract -- reuse, reaping a dead client, retaining ownership on context exit, idempotent close_all -- is stated by its tests instead of implied by adapter behaviour. arc/job/ssh_pool_test.py drives SSHConnectionPool with a stub factory and covers the cases adapter_test.py could not reach, namely that a raising with-body leaves the pool reusable and that reset_default_pool() closes pooled clients rather than just dropping the reference. The adapter-driven integration tests stay with the adapter, which is what they actually exercise. Two claims the pool's docstrings make were still untested, and the imports for them were sitting unused in adapter_test.py: that a remote-queue execute() with no pool injected borrows from the instance get_default_pool() returns, and that reset_default_pool() -- ARC.py's exit hook -- closes the clients those jobs opened and leaves a usable empty pool behind. Both are now asserted rather than implied. The pool tearDowns also called set_default_pool(None), which drops the reference without closing anything, so each test class leaked its stub clients and contradicted the lifecycle ssh_pool.py documents; they call reset_default_pool() instead. Also two test-only cleanups CodeQL flags: two factory lambdas that only forwarded their argument now pass the callable itself, and the "a raising with-body leaves the pool usable" test uses assertRaises' callable form. Its context-manager form made every statement after the block unreachable to a control-flow analyser, because nothing in the CFG says assertRaises.__exit__ suppresses the exception. Two of CodeQL's remaining alerts on this file are in adapter_test.py and are the same two defects already fixed in ssh_pool_test.py. The assertRaises context-manager form around a with-block whose last statement is `raise` makes everything after the block unreachable to a control-flow analyser, since nothing in the CFG says __exit__ suppresses the exception; both tests use the callable form, and still assert what they did -- that the caller's own exception reaches the caller, and that the pool is usable afterwards. And the module both imported arc.job.adapter and imported names from it; the module-alias form existed for two patch.object() calls, which are now patch('arc.job.adapter.<name>'), so the file uses one import form. Dropping the alias also removes a name that a class attribute in the same file shadowed. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: set_file_paths() splits the project's remote directory out of the job's remote path as remote_project_path, which is what the remote check file cleanup is scoped to.
calvinp0
added a commit
that referenced
this pull request
Aug 20, 2026
…etry Four related defects in SSHClient._connect(), all on the authentication path: The configured 'key' was never used as an identity. connect() was called without key_filename, while load_system_host_keys() was given the private key path -- but that function expects a known_hosts database, so the private key parsed as an empty host-key set and was never offered for authentication. A server configured with an explicit 'key' therefore authenticated only by accident, via an agent or a key already in a default location, and otherwise failed in a way that looks like a server or a network problem. Load the system host keys from their default location and pass the configured key to connect() as key_filename. Once the key is a real identity it must name a real, readable file: paramiko raises when key_filename points at a path that does not exist. That would exclude every setup which authenticates without a key file on the ARC machine, and forwarding an agent into a container is exactly such a setup -- the one where the key file deliberately never exists locally. key_filename=None is valid and makes paramiko fall back to the agent and then to the default ~/.ssh/id_rsa, id_ecdsa and id_ed25519 paths (no id_dsa, which paramiko 4 dropped), so read 'key' with .get() and treat an empty string as unset. A configured key is still forwarded unchanged. This does not extend to ~/.ssh/config: paramiko honours it only when the application constructs a paramiko.SSHConfig itself, and ARC never does, so ProxyJump and bastion hosts remain unsupported. _connect()'s docstring now says so, making that limitation discoverable from the code rather than only from a failed connection. AutoAddPolicy() accepted and stored any unseen host key without a word, so a first connection and a machine-in-the-middle were indistinguishable and equally silent. Default to LogAndAcceptHostKeyPolicy, which still connects but says so, and add a per-server 'strict_host_key_checking' flag selecting RejectUnknownHostKeyPolicy for deployments that want unknown hosts refused outright. Warning is the default because rejecting would break any host not already in known_hosts, which is a behaviour change users should opt into rather than inherit. Both policies are ARC's own, because paramiko's are unusable here. WarningPolicy reports through warnings.warn(), and initialize_log() installs filterwarnings(action='ignore', module='.*paramiko.*'), so its message reached nobody -- measured, one warning before the filter and none after; ARC's policy logs the host and the key's OpenSSH-style SHA256 fingerprint at warning level and then accepts, leaving the connect-anyway behaviour exactly as it was. RejectPolicy raises a plain SSHException, which is indistinguishable from a transport failure without matching on its message text, so the strict policy raises UnknownHostKeyError instead -- an ARC ServerError, and also a paramiko.SSHException since that is what a missing host key policy is expected to raise, so the retry loop catches it by type and ARC's existing server error handling covers it everywhere else. The retry around connect() used a bare 'except'. That also caught KeyboardInterrupt and SystemExit -- so ctrl-C during a connection attempt silently retried instead of aborting -- and it discarded the first exception, meaning a bad key or a wrong username surfaced as whatever the retry happened to raise. Catch (paramiko.SSHException, OSError), which covers the documented "Error reading SSH protocol banner / Connection reset by peer" flake the retry exists for, and log the first failure so its cause is not lost. That retry is also the wrong answer to a permanent failure. connect() wraps _connect() in a 1440 x 60 s loop and _connect() retries once, so a rejected key or a wrong user name cost roughly 2880 authentication attempts spread over 24 hours against a server that was never going to accept them -- the traffic shape that gets an account locked out. Classify first: AuthenticationException and BadHostKeyException (both SSHException subclasses) and UnknownHostKeyError are permanent, and are raised on the first attempt as a ServerError carrying the paramiko exception as its cause, so connect()'s documented failure type is unchanged. Everything else is transport-level and keeps the retry. Also retry the remote-file existence check a few times before reporting a missing download -- PBS/SGE epilogues can flush stdout/stderr a second or two after qstat reports the job has left the queue, and the previous code warned on that race -- and surface a hint on memory-related submission failures. When the remote file really is absent, the local path is emptied rather than left as it was. paramiko's SFTPClient.get() opens the local path 'wb' before it reads the remote file, so main's fall-through created or truncated it on every miss; returning early instead would leave a previous job's out.txt in place, and JobAdapter._get_additional_job_info() reads whatever out.txt and err.txt are on disk into additional_job_info, attributing an earlier job's server output to this one. Truncating rather than deleting reproduces main's observable state exactly: those readers gate on os.path.isfile and then read, so a zero-byte file and an absent file are not the same thing to them, and changing which one they see is not this change's business. That miss is now reported at warning level rather than debug. A job with no stdout at all is a strong signal of an abnormal failure -- the scheduler killed it, the submit script never copied results back from scratch, or ARC is looking at the wrong remote directory -- and ARC has no branch for that condition today: adapter.py's `if self.additional_job_info:` simply does nothing when it is empty, so this line is the only trace that it happened. Raising the level adds no handling, it stops the event being invisible while handling is designed separately. Two follow-ups belong to that separate work and are deliberately not here: classifying why the output is absent (a remote directory listing, then scheduler accounting for the job id, then a distinct no_output status), and a guard that stops a run after N consecutive jobs produce no output. The three-attempt existence check itself is unchanged -- same count, same one second interval, same short-circuit as soon as the file appears -- and is now pinned by tests, including one where the file shows up on the second attempt and is downloaded normally. The commented 'pharos' example in the settings file gave a known_hosts path as 'key'; with these semantics 'key' is the private key, never the host-key database, so that is dropped. Verified against a live server: both host-key policies connect successfully with the host already in known_hosts, and _connect() opens SSH and SFTP sessions and closes cleanly. Warning is also the default because rejecting does not fail a scheduler once. A refused host key fails every submission, status poll and download for that server while the ARC driver stays alive, so the run continues and produces nothing; the refusal raises the same exception class as a transport error, so it also feeds the 24-hour connection retry loop and presents as a hang. Recovery means stopping ARC, running ssh-keyscan and restarting, hours after the fact. The rationale now lives in _connect()'s docstring rather than in line comments, so it is discoverable from the code. That default is only defensible if the risk it accepts is visible, so check_servers_known_hosts() reports, at startup and before any calculation is submitted, every configured server whose address has no host key in ~/.ssh/known_hosts -- naming the server, the address and the ssh-keyscan command that seeds it. It is a warning, never fatal, consistent with the connection policy itself, and it is entirely local: paramiko's HostKeys does the lookup, so hashed (`ssh-keyscan -H`) and [host]:port entries match as OpenSSH matches them, and nothing is resolved and no connection opened. The 'local' server, entries with no address, and entries still carrying the repository's placeholder *.host.edu address or <username> user name are skipped, since reporting them would fire on every run made with the default settings. arc/main.py calls it once logging is initialized and before determine_ess_settings(), so it covers a plain local run and not only the container, where dockerfiles/arc_preflight.py already checked a coarser version of the same thing. The connect test helper caught BaseException, which CodeQL flags and which would have reported an unrelated interpreter-level error as an expected one; it now catches only the exception types the tests assert on. A host key that is present and contradicted is a different event from one that is absent, and it was reported as neither. BadHostKeyException was classified as permanent and raised as a plain ServerError, so a machine-in-the-middle read in the log exactly like a wrong password. connect() now raises HostKeyMismatchError -- a ServerError, so nothing that handles server errors changes, and a paramiko.SSHException like the exception it is raised from -- and reports it at the error level, naming the stored and the presented fingerprints, because which of the two the reader recognises is what decides whether the server was re-keyed or the session was intercepted, and giving the ssh-keygen -R that replaces the stored key once the new one is confirmed. That comparison needs the server, so it cannot be part of the startup report, and get_servers_missing_host_keys() says so rather than leaving the reader to assume otherwise. What a known_hosts file can be asked on its own is whether it contradicts itself: get_servers_with_conflicting_host_keys() reports an address with more than one entry of the same key type. A host legitimately has one key per type and ssh-keyscan writes one line per type, so a second is either a stale key left by a rebuilt server or one placed there to shadow the real one -- and only the first matching entry is ever consulted, by OpenSSH and by the HostKeys.lookup paramiko authenticates with, so the shadowed key is trusted silently while the server's own key is then reported as a mismatch. It is reported at the error level, unlike an absent key, and the two checks share their server filter and their known_hosts load rather than repeating them. check_connections() did `self._sftp, self._ssh = self.connect()`, but connect() assigns both itself and returns None, so that branch raised `TypeError: cannot unpack non-iterable NoneType object` for exactly the case it exists to serve, a client that has not connected yet. Nothing hit it while every caller used `with SSHClient(...)`, which connects in __enter__ -- except trsh_job_on_server, whose bare `ssh = SSHClient(server)` meant that troubleshooting a remote job by changing node could not work at all. Call connect() for its effect. ARC's ESS survey opened one connection per server and dropped it, immediately before the scheduler needed a connection to the same servers; it borrows from the pool instead, so that connection is the one the run's jobs then use. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: SSHClient gains a configurable connection_attempts, delete_remote_check_files() and the module-level delete_check_files_on_servers(), and ARC.clean_check_files() calls both halves of the cleanup at the end of a run, so the checkfiles a run leaves on a server are deleted rather than only the local ones. Four places where that work and this branch's own changes to ssh.py met were resolved rather than taken from one side: * shlex is imported once; this branch already imported it to quote remote commands. * __init__ keeps this branch's `servers[server].get('key') or None`, not #1000's `servers[server]['key']`. An optional key is this branch's point, and #1000's line was written against the older mandatory one. * connect() keeps this branch's permanent-failure classification and takes #1000's configurable attempt count. They are independent: the classification decides whether a failure is worth retrying at all, connection_attempts decides how long retrying goes on. A permanent failure still raises on the first attempt whatever connection_attempts is set to. * The new methods are placed by subject rather than at #1000's line offsets: delete_remote_check_files() next to remove_dir(), the other remote deletion primitive, and delete_check_files_on_servers() with the other module-level server helpers. delete_remote_check_files() builds a shell command, so its remote path is quoted with shlex.quote() like every other interpolation in this module.
calvinp0
added a commit
that referenced
this pull request
Aug 20, 2026
The pool was built and tested but the highest-frequency caller never used it. Scheduler.get_server_job_ids() opens a connection per server per poll cycle, for every cycle of every job's lifetime -- on a run of any length that is the dominant source of connections by an order of magnitude, and it is exactly the traffic shape a per-user connection limit is there to stop. It borrows now, so a run's polling costs one connection per server rather than one per poll. The same for the three sites in trsh_job_on_server() and for CFour's execute_queue(). CFour overrides execute_queue() rather than calling JobAdapter.legacy_queue_execution(), so it did not inherit the sharing the other adapters got; it goes through _open_or_borrow_ssh(), which means its submission also reuses the client its upload just used. One of those trsh sites leaked. `ssh = SSHClient(server)` with no `with` and no close() left a connection open for the rest of the process every time a job was troubleshooted by changing node. It never actually reached the server, since check_connections() raised TypeError on an unconnected client (fixed with the rest of the SSH work), but the leak is real for any caller that got past it. Not routed: delete_all_arc_jobs() in arc/job/ssh.py. Its only caller is arc/utils/delete.py, a standalone command-line utility that deletes jobs and exits, outside any ARC run; it opens no connection ARC would otherwise reuse and its `with` already closes what it opens, so pooling would swap a closed connection for one left open until the interpreter exits. ssh.py is also the module ssh_pool.py imports, so pooling there would have to be a function-local import to avoid a cycle -- a cost with nothing bought. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: the scheduler records the remote project path of each server it spawns a job on, which is what the end of a run hands to the check file cleanup.
calvinp0
added a commit
that referenced
this pull request
Aug 20, 2026
PipeRun.submit_to_scheduler() invokes qsub/sbatch on the machine running ARC, and the worker (python -m arc.scripts.pipe_worker) reads pipe_root from its local filesystem. When the engine's resolved server is remote, that submission errors silently and the run deadlocks waiting for results that can never arrive. Make should_use_pipe() refuse a non-local server so the planner falls back to per-job queue submission over SSH, and say in the log which engine and server triggered the refusal and what is being used instead -- that fallback is slower than a pipe run, so without the message the only symptom is an unexplained slowdown. Supporting pipe on a remote server needs it rebuilt around batch jobs staged on the remote side, which is out of scope here. The guard resolved its server with `next((s for s in server_list if s in servers_dict), None)`, which fails open in three ways: it skips an entry that names an unconfigured server and silently judges the next one instead, it compares server names case-sensitively when a server name is a settings key whose casing the user chose, and it permits the pipe when nothing resolves at all. That last one matters most, because derive_cluster_software() applies the same "skip what is not configured" rule and then falls back to guessing slurm, so an unresolvable server produced a pipe submitted with a guessed template. Resolve the first entry unconditionally, compare case-insensitively, and refuse unless the result is a configured server that is this machine. Refusing costs the run only the bundling -- the planner submits the tasks as individual queue jobs, which works for a local and a remote server alike -- so failing closed here is cheap and failing open is not. "Cannot be resolved" is not the same as "has no server", and conflating the two would have disabled TSG pipe mode outright. A TS-guess batch carries engine=<method>, and gcn, kinbot, xtb_gsm and the rest are not ESS: they are absent from ess_settings by design and run in this process, which is why _initialize_adapter resolves a server only for an engine ess_settings names and leaves every other one with server=None, and why set_file_paths gives such a job no remote path at all. In process is this machine, so those tasks pipe. The refusal is for an engine that ess_settings does name and that still does not resolve to a configured local server -- an ESS declared and available nowhere, or named on a server that is not configured. The resolution itself is not a second implementation. _initialize_adapter() already decided which server a job goes to, inline: a trsh override first, then the first entry of the ESS settings for the adapter, with a bare string read as a single server. That is now resolve_job_server() in arc/job/adapters/common.py, the module that owns the concept, called by both, so the pipe's answer is the answer the job would have got rather than a lookalike. Extracting it also fixes an IndexError on an empty server list, and drops a redundant re-check of a condition the enclosing `if` had already established. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: _initialize_adapter() initializes the new remote_project_path attribute.
calvinp0
added a commit
that referenced
this pull request
Aug 20, 2026
The container and SSH halves of remote submission were in place; what was missing was the user-facing configuration around them. These docs are written against the `key` and host-key semantics this branch introduces, not against main's. docs/source/remote_submission.rst (new, in the toctree): authentication via a forwarded agent (preferred -- keys never enter the container and passphrase-protected keys keep working) or via a mounted key file; host key verification and the new per-server strict_host_key_checking, including why an unseeded known_hosts matters in a fresh container now that WarningPolicy and RejectPolicy have replaced the silent AutoAdd; the ~/.arc overlay mount, which a remote run needs as much as the SSH material since submit.py carries the cluster's PBS/Slurm templates; both `docker run` invocations and the compose equivalent; and the entrypoint's exit codes. Two limitations are documented rather than worked around: - ARC never builds a paramiko.SSHConfig, so ~/.ssh/config is not read at all and ProxyJump/bastion hosts are unsupported. This is true on bare metal too, and is called out so nobody blames the container for it. - a default-bridge container reaches an ordinary login node with no extra flags, since paramiko speaks SSH itself; the exceptions are a host VPN whose routing excludes docker0, and internal names served only by a VPN-pushed resolver. Each claim is checked against the code rather than assumed: paramiko's load_system_host_keys() reads ~/.ssh/known_hosts and nothing else, so /etc/ssh/ssh_known_hosts is not mentioned as an alternative (seeding it under strict_host_key_checking would have refused every connection); a rejected host key raises into the same 24-hour retry loop and so presents as a hang rather than a fast failure; the retry reason reaches the logger only on every tenth attempt, the others going to stdout; and Docker materialises a missing bind-mount source as a root-owned directory, which is what a stale SSH_AUTH_SOCK or an absent ~/.arc produces on the host. installation.rst and running.rst described `key` as a private key path, which was wrong on main and is right as of this branch; they now say so, present the agent route as the default, and mention strict_host_key_checking. docker.rst gains the remote-submission pointer, index.rst the toctree entry, and the stray `key` in the advanced.rst node-limits example is dropped, since that example is about cpus and memory. remote_submission.rst now also states why warning rather than rejecting is the default host-key policy -- a refused key does not fail a long-running scheduler once but starves it while the driver stays alive, and it presents as a hang rather than an error -- and how to opt into refusal per server. The Startup Checks section leads with the check ARC itself performs on every run, in a container or not, and describes the compose file's known_hosts mount and why ARC_KNOWN_HOSTS defaults to /dev/null. running.rst gains a pointer to the same startup report. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: keep_checks is documented as covering the servers a project ran on, not only the local project directory.
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
2 participants
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.
ARC uploads a Gaussian checkpoint as a wavefunction guess for each job and downloads it again afterwards. When a run terminates it deletes those files — but only on the local machine:
delete_check_files(project_directory)walks<project_directory>/calcs;main.pycalls it unconditionally at the end of a run, sincekeep_checksdefaults toFalse;So the documented policy — "they usually take up lots of space and are not needed after ARC terminates" — is half-implemented, and every checkpoint ARC has ever uploaded is still sitting in the remote project directory.
Measured impact. On a quota-managed cluster, 82.3% of a fully characterised species' 61.4 MiB footprint is exactly this class of file — the next largest class is 5.6× smaller — and 6.35 GiB had silently accumulated against a 300 GB quota. Nothing downstream reads the remote copy: thermochemistry is parsed from the primary output log, and the local copy is the one reused as a guess.
What this does. Adds
SSHClient.delete_check_filesand adelete_remote_check_fileshelper, and routes the existing teardown through a newARC.clean_check_files. Design constraints kept deliberately tight:keep_checks=Truekeeps them everywhere, local and remote — the flag's meaning is unchanged;*.chkunder the run's own remote project directory, using the same path constructionjob/adapter.pyalready uses, and no-ops if that directory does not exist;localare skipped; nothing about uploads, the wavefunction-guess mechanism, orkeep_checks's default changes.Tests cover all three: deleted with
keep_checks=False(asserting only.chk, and only under this project's directory), kept withkeep_checks=True, and a run completing normally when the server call raises. Verified as a regression test rather than a tautology — removing just the remote call while leaving the helper in place makestest_check_files_are_deleted_locally_and_remotelyfail.