Skip to content

Only process - #653

Open
alongd wants to merge 3 commits into
mainfrom
only_process
Open

alongd wants to merge 3 commits into
mainfrom
only_process

Conversation

@alongd

@alongd alongd commented May 10, 2023 •

Copy link
Copy Markdown
Member

We'd like ARC to immediately process thermo/kinetics after all relevant jobs have ended.
This PR takes us one small step closer to this goal, adding an only_process arg to the input (restart) file, to request ARC to only process what it can and not spawn new jobs in this restart mode when only_process is True.

Also, save the species adjlist if given. Some species are not read correctly in RMG (especially hyperventilate N and S containing species), which results in excluding them from the final thermo library. Here we use the user-provided adjlist (which is the best way to define these species) if given.

Comment thread arc/plotter.py Fixed
Comment thread functional/functional_test.py Fixed
Comment thread functional/functional_test.py Fixed
Comment thread arc/species/zmat.py Fixed
Comment thread arc/species/zmat.py Fixed
Comment thread arc/job/adapters/ts/kinbot_test.py Fixed
Comment thread arc/common.py Fixed
@codecov

codecov Bot commented May 14, 2023 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.10%. Comparing base (828e7a6) to head (6c63564).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
- Coverage   67.18%   67.10%   -0.08%     
==========================================
  Files         123      123              
  Lines       43674    43694      +20     
  Branches    11153    11160       +7     
==========================================
- Hits        29342    29323      -19     
- Misses      11194    11225      +31     
- Partials     3138     3146       +8     
Flag Coverage Δ
functionaltests 67.10% <ø> (-0.08%) ⬇️
unittests 67.10% <ø> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 8 files with indirect coverage changes

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

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 PR introduces an "only process" mode to ARC, allowing users to skip job spawning and only process existing calculations when restarting. The implementation adds an only_process parameter to control whether new jobs should be spawned, and improves species handling by preserving user-provided adjacency lists for better accuracy with complex nitrogen and sulfur species.

  • Adds only_process parameter to ARC and Scheduler classes to control job spawning behavior
  • Preserves user-provided adjacency lists in species definitions for better molecular representation
  • Updates test cases to include arkane_file attribute in expected output dictionaries

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
arc/scheduler.py Adds only_process parameter to Scheduler class and prevents job spawning when enabled
arc/main.py Adds only_process parameter to ARC class and passes it to Scheduler
arc/reaction/reaction_test.py Updates test expectations to include arkane_file attribute in species dictionaries

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread arc/scheduler.py

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

kfir4444 added a commit that referenced this pull request Oct 4, 2026
…rift (#1066)

## What

Adds a `codecov.yml` (23 lines, no code change):

- `coverage.status.project.default.threshold: 1%`
- `coverage.status.patch.default.target: 80%`

## Why

ARC has no Codecov configuration at all — `.coveragerc` configures
coverage.py's *collection*, not
Codecov's *status checks*. With no config, Codecov falls back to its
built-in defaults, under which
the `project` status is `target: auto` with a **0% threshold**: total
coverage may never dip by any
amount relative to the base commit.

The consequence is that nearly every PR adding code covered below the
repository average (~67%)
reports a failing check while every test job, CodeQL and the functional
suite are green. Observed
on the current open set, all red on `codecov/project` alone:

| PR | status |
|---|---|
| #653 | `67.03% (-0.10%)` → `67.05% (-0.08%)` after adding a genuine
unit test |
| #860, #1016, #1039, #1050, #1054, #1055, #1056, #1057 | red on
`codecov/project`, everything else green |

On #653, `codecov/patch` simultaneously reported *"Coverage not
affected"* — the gate that actually
judges whether the PR's new code is tested was already satisfied. A
check that fails on
one-tenth of a percentage point trains reviewers to ignore it, which
costs us the patch signal too.

## What this does and does not change

- **Does:** lets total coverage drift up to 1% without failing the
status.
- **Does:** keep an explicit `patch` target, so the coverage of a PR's
own new or changed lines
remains a gating signal. This is the stricter half of the config, not a
loosening.
- **Does not:** touch any test or source file, change `.coveragerc`, or
alter what coverage is
  collected.
- **Does not:** raise ARC's actual coverage. That is a separate and much
larger piece of work.

Note that Codecov re-evaluates statuses on new uploads, so already-open
PRs will pick this up on
their next CI run rather than retroactively.
@alongd
alongd requested a balanced review from Copilot October 19, 2026 03:15

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.

🟡 Changes recommended

Monoatomic restarts lose restored job tracking, and processing-only mode can still submit frequency-calibration jobs.

2 open issues

Review effort: Balanced

Comment thread arc/main.py
Comment thread arc/scheduler.py Outdated

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.

🟡 Changes recommended

Processing-only runs can skip completed results and delete checkpoints needed by unfinished jobs.

2 open issues
2 resolved since last review

Review effort: Balanced

Comment thread arc/main.py
Comment thread arc/scheduler.py Outdated
only_process was added with no test exercising it. Covers the arc/main.py side:
stored on the instance, written to the restart dict only when truthy (so existing
restart files are unchanged), and accepted back as an input key.

Also covers the two paths a Copilot review found unguarded:

- processing-only mode must not calculate a missing frequencies scaling factor.
  determine_scaling_factors() builds a Scheduler of its own that never sees
  only_process, and it is reached from ARC.__init__, so it would submit
  calibration jobs for small species before execute() ever reaches the guarded
  project scheduler. The test asserts it is not called, and asserts the same
  input without only_process does call it, so the guard is what stops it.
- a monoatomic species' restored sp job must still be tracked after a restart.
  Its job_dict entry suppresses a replacement, so an un-tracked job leaves the
  species unconverged for the rest of the run.

The schedule_jobs() guard itself stays uncovered: testing=True already
short-circuits it.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants