Skip to content

Report per-run cost metrics in output.yml - #945

Open
calvinp0 wants to merge 3 commits into
mainfrom
feature_cost_metrics
Open

calvinp0 wants to merge 3 commits into
mainfrom
feature_cost_metrics

Conversation

@calvinp0

@calvinp0 calvinp0 commented Aug 2, 2026 •

Copy link
Copy Markdown
Member

Stacked on #917 (feature_arc_result_export_contract), stack #946. This PR's diff shows only its own layer.

#945  Report per-run cost metrics in output.yml   ← you are here
 └── #917  Export tool-neutral ARC results        (base: main)

It depends on #917 rather than merely following it: cost_metrics is a section of the output.yml schema that #917 defines, and it extends write_output_yml()'s existing t0 parameter. It cannot land first.

What this adds

A cost_metrics section in output.yml, answering "what did this run actually cost?":

cost_metrics:
  wall_time_hrs: 26.4
  total_job_count: 118
  total_execution_time_hrs: 402.1
  total_core_hours: 3216.8
  jobs_missing_time: 0
  jobs_missing_cores: 2
  per_ess:
    gaussian: {job_count: 94, execution_time_hrs: 380.2, core_hours: 3041.6, jobs_missing_time: 0}
    molpro:   {job_count: 24, execution_time_hrs:  21.9, core_hours:  175.2, jobs_missing_time: 0}

Three layers, one per commit:

  1. arc/scheduler.py — the Scheduler appends a lightweight record for every job reaching end_job (name, label, job type, adapter, server, cpu cores, run time, status). Pipe-mode tasks bypass the JobAdapter path and carry no per-task timing, so they are deliberately not recorded; only jobs ejected back to the Scheduler are.
  2. arc/output.py — _compute_cost_metrics() aggregates them; _timedelta_to_seconds() normalizes the several shapes a run time arrives in (timedelta, numeric seconds, H:MM:SS, D days, H:MM:SS), returning None rather than raising on an unparseable value.
  3. arc/main.py — wires the records through to write_output_yml(), and accepts them back on restart.

Incomplete accounting is counted, not hidden

Jobs with no recorded run time or core count land in jobs_missing_time / jobs_missing_cores instead of being silently dropped, so a consumer can tell how complete the numbers are. A run where a third of jobs lack timing produces a visibly partial total rather than a confidently wrong one.

The restart half is a real bug fix, not plumbing

Persisting the records is what makes them survive a restart — and that is exactly where this bit us in practice:

TypeError: ARC.__init__() got an unexpected keyword argument 'completed_job_records'

ARC.py restarts with ARC(**read_yaml_file('restart.yml')), so the restart file is ARC's constructor namespace — but it is ARC.as_dict() plus whatever Scheduler.save_restart_dict() adds to it. Writing the records there without a matching constructor parameter makes every restart of a project that has accumulated any fail instantly. as_dict() now emits the key when non-empty so the round trip is symmetric, and a restart file written before the key existed still constructs.

TestRestartRoundTrip asserts that every key written into a restart dictionary — by as_dict() and by the Scheduler — is a constructor parameter. That invariant had no coverage at all: ARC(**read_yaml_file(restart.yml)) was exercised by no test, which is why this class of breakage stays invisible until somebody actually restarts. Any future key added to Scheduler.save_restart_dict() without touching main.py now fails a test instead of a multi-day run.

Evidence

222 tests pass (output_test, main_test, scheduler_test), including 20 new ones: _timedelta_to_seconds across all input shapes, _compute_cost_metrics aggregation / missing-data counting / unknown-adapter bucketing, the cost_metrics section end-to-end through write_output_yml, and the four restart round-trip guards.

Notes for review

  • Each commit touches a disjoint set of files (scheduler.py | output.py+tests+docs | main.py+tests), so the three are independently readable.
  • write_output_yml()'s new completed_job_records defaults to None, alongside the existing t0; a caller supplying neither gets an empty cost_metrics block rather than a missing key.
  • Downgrading ARC after a restart file exists. A restart.yml written by this branch carries completed_job_records, which an ARC predating it cannot accept: ARC.py splats the file straight into the constructor and neither signature takes **kwargs, so running an older ARC against it raises the same TypeError above, naming the new key. The key is only written once at least one job has completed, and deleting that one line from restart.yml restores compatibility. The upgrade direction — the one that actually happens — is safe and covered: a restart file written before the key existed still constructs, and this branch removes no constructor parameter.
  • Scoped deliberately: this is the cost-metrics half only. The TS-guess provenance that shipped alongside it in our internal branch is not included — Export tool-neutral ARC results and parser evidence, and fix three Molpro defects #917 already has its own ts_guesses export, and that is the version kept here.

@calvinp0 calvinp0 changed the title feature cost metrics Report per-run cost metrics in output.yml Aug 2, 2026
@codecov

codecov Bot commented Aug 2, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.05%. Comparing base (cefbbad) to head (4c35867).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #945      +/-   ##
==========================================
- Coverage   67.06%   67.05%   -0.02%     
==========================================
  Files         123      123              
  Lines       43597    43653      +56     
  Branches    11143    11154      +11     
==========================================
+ Hits        29240    29273      +33     
- Misses      11212    11234      +22     
- Partials     3145     3146       +1     
Flag Coverage Δ
functionaltests 67.05% <ø> (-0.02%) ⬇️
unittests 67.05% <ø> (-0.02%) ⬇️

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.

@calvinp0
calvinp0 force-pushed the feature_cost_metrics branch from 76abfa2 to 90e682b Compare August 2, 2026 20:19
@calvinp0
calvinp0 force-pushed the feature_cost_metrics branch from 90e682b to b9b701a Compare August 8, 2026 09:45
@calvinp0
calvinp0 marked this pull request as ready for review August 8, 2026 09:48
Copilot AI lite review requested due to automatic review settings August 8, 2026 09:48

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 was unable to review this pull request because the user who requested the review has reached their quota limit.

@calvinp0
calvinp0 force-pushed the feature_cost_metrics branch from b9b701a to 65adf9b Compare August 8, 2026 12:40
@calvinp0
calvinp0 force-pushed the feature_cost_metrics branch from 65adf9b to b01ccf0 Compare August 8, 2026 15:12
@calvinp0
calvinp0 force-pushed the feature_cost_metrics branch from b01ccf0 to 575c947 Compare August 8, 2026 15:22
@alongd
alongd requested a lite review from Copilot August 8, 2026 18:42

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

arc/main_test.py:581

  • The completed_job_records fixture here uses keys (adapter, run_time, status) that don't match the record schema produced by Scheduler._record_completed_job() / consumed by _compute_cost_metrics() (job_adapter, run_time_sec, job_status, plus label). Keeping the test data in the real schema will make the restart round-trip coverage more representative and avoids accidental drift in key naming.
        records = [{'job_name': 'opt_a1', 'job_type': 'opt', 'adapter': 'gaussian',
                    'server': 'local', 'cpu_cores': 8, 'run_time': 12.5, 'status': 'done'}]

@calvinp0
calvinp0 force-pushed the feature_cost_metrics branch 4 times, most recently from 1b241de to 0a9947b Compare August 10, 2026 09:15
@calvinp0
calvinp0 force-pushed the feature_cost_metrics branch from 0a9947b to 5a73846 Compare August 10, 2026 19:36
@calvinp0
calvinp0 force-pushed the feature_cost_metrics branch from 5a73846 to 8966314 Compare September 24, 2026 11:47
Base automatically changed from feature_arc_result_export_contract to main September 26, 2026 20:47
The Scheduler now appends a lightweight record for every job that reaches `end_job`:
job name, species label, job type, adapter, server, cpu cores, run time in seconds, and
final status. The records accumulate on `Scheduler.completed_job_records` and are
persisted in the restart file so a restarted run keeps its cost history.

Pipe-mode tasks bypass the JobAdapter path and carry no per-task timing, so they are not
individually recorded; only jobs ejected back to the Scheduler are.
Adds a `cost_metrics` section to output.yml, aggregating the per-job records the
Scheduler collects into wall time, total job count, total execution time, total core
hours, and a per-ESS breakdown. Jobs whose run time or core count could not be
determined are counted in `jobs_missing_time` / `jobs_missing_cores` rather than being
silently dropped, so a consumer can tell how complete the accounting is.

`_timedelta_to_seconds()` normalizes the several shapes a run time arrives in
(`timedelta`, numeric seconds, `H:MM:SS`, `D days, H:MM:SS`), returning None rather than
raising on an unparseable value.

`write_output_yml()` gains a `completed_job_records` argument alongside the existing
`t0`; both default to None, so a caller that supplies neither gets an empty
`cost_metrics` block rather than a missing key.
`ARC` hands the Scheduler's records to `write_output_yml()` so the run's `cost_metrics`
are populated, and accepts them back as a constructor argument so they survive a restart.

That second half matters more than it looks. `ARC.py` restarts a project with
`ARC(**read_yaml_file('restart.yml'))`, so the restart file is also ARC's constructor
namespace - but it is `ARC.as_dict()` plus whatever `Scheduler.save_restart_dict()` adds
to it. Persisting the records without a matching constructor parameter therefore makes
every restart of a project that has accumulated any fail immediately with
`TypeError: got an unexpected keyword argument`. `as_dict()` emits the key when non-empty
so the round trip is symmetric, and a restart file written before the key existed still
constructs.

`TestRestartRoundTrip` asserts that every key written into a restart dictionary - by
`as_dict()` and by the Scheduler - is a constructor parameter. That invariant previously
had no coverage at all: `ARC(**read_yaml_file(restart.yml))` was exercised by no test, so
this class of breakage stayed invisible until somebody restarted an affected project. Any
future key added to `Scheduler.save_restart_dict()` without touching `main.py` now fails
a test instead of a run.
@alongd
alongd force-pushed the feature_cost_metrics branch from 8966314 to 4c35867 Compare September 26, 2026 20:47

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.

2 participants