Report piped species as converged when their sp/freq jobs succeed - #934
Open
calvinp0 wants to merge 2 commits into
Open
Report piped species as converged when their sp/freq jobs succeed#934calvinp0 wants to merge 2 commits into
calvinp0 wants to merge 2 commits into
Conversation
A piped species_sp or species_freq task computed correctly, wrote valid output and was ingested, but was still reported as failed: the ingestion helpers record the parsed results without setting output[label]['job_types']['sp'|'freq'], and those flags are only ever set by the Scheduler's own post-job logic on the non-piped path. ARC then declared every piped species unconverged and refused to compute rate coefficients. The flags are not merely "the job finished" markers, so they are not set directly here. job_types['freq'] stands for the imaginary-frequency QA (exactly zero for a stable species, exactly one for a TS), and job_types['sp'] is set only at the end of post_sp_actions, after the T1 diagnostic, solvation-scheme follow-ups and the reaction E0 check. Setting the booleans directly would have turned a fail-safe bug, where good results are discarded, into a fail-open one where a stable species carrying an imaginary frequency is accepted. Piped tasks are therefore routed through the same Scheduler methods the individually submitted ones use. post_sp_actions() is already path-based and needs no adaptation; check_negative_freq() reads only local_path_to_output_file from its job argument, so a minimal PipedJobView supplies it. Finalization runs per task inside the ingestion loop rather than in _post_ingest_pipe_run, because a cross-species batch carries one species per task and because _post_ingest_pipe_run is skipped entirely when any sibling task is ejected for troubleshooting. A task can reach COMPLETED while still holding a non-converged ESS output, so the convergence gate is repeated before any flag can be set. _check_ess_convergence is renamed to check_ess_convergence now that it has a caller in a sibling module. This path is reached only when a species_sp/species_freq batch actually forms, which needs at least pipe_settings['min_tasks'] (default 10) homogeneous species-level tasks in one flush. Ordinary runs fall back to individual jobs and are unaffected, which is why this went unnoticed.
Setting the sp/freq success flags for piped tasks is not enough on its own: the species also has to still be reachable when its pipe run terminates. check_all_done() is only called from the main loop while iterating running_jobs, and a species whose jobs were routed to pipe mode holds no entries there. Its empty entry was deleted on the first iteration after the batch was submitted, even though has_pending_pipe_work() had just reported the species as busy, so once the pipe run finished the label was gone, the loop never revisited it, and convergence stayed False for a species whose every job succeeded. The deletion now happens only on the branch that has already established there is no pending pipe work. The main loop keeps running in the meantime because its condition already accounts for active_pipes. The pipe-pending predicate moves into has_pending_pipe_work() so that the reason an empty running_jobs entry cannot be dropped is stated where the rule lives, and so it can be tested directly. The new test is its own TestCase rather than a TestScheduler method: it needs no fixture beyond a stub, and TestScheduler shares class-level state across its tests, which already fails under xdist when the tests are split across workers.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #934 +/- ##
==========================================
+ Coverage 63.45% 63.48% +0.03%
==========================================
Files 114 114
Lines 38325 38358 +33
Branches 10030 10033 +3
==========================================
+ Hits 24319 24352 +33
- Misses 11089 11096 +7
+ Partials 2917 2910 -7
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:
|
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
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.
The bug
A
species_sporspecies_freqtask routed through pipe mode computes correctly, writes valid output, passesdetermine_ess_statusand is ingested — and the species is then reported as failed, aborting the rate coefficient:The data is all present. From
restart.ymlof that run:e_elect: -106300.678andpaths['freq']: True, sitting next tojob_types: {'freq': False, 'sp': False}.There are two independent causes.
1. Nothing sets the success flags.
_ingest_species_sp()setsspecies.e_electand_ingest_species_freq()setsoutput[label]['paths']['freq'], but neither setsoutput[label]['job_types']['sp'|'freq']. Those are only ever set on the non-piped path inscheduler.py._ingest_species_sp()also never setspaths['sp'].2. The species stops being reachable.
check_all_done()is only called from the main loop while iteratingrunning_jobs, and a piped species holds no entries there. Its empty entry was deleted on the first iteration after the batch was submitted — even thoughhas_pending_pipe_work()had just reported it busy — so when the pipe run finished the label was gone andconvergencestayedFalse.Why the flags aren't just set directly
job_types['freq']is not a "job finished" marker. It stands for the imaginary-frequency QA — exactly zero for a stable species, exactly one for a TS, withswitch_ts()on failure.job_types['sp']is set only at the end ofpost_sp_actions(), after the T1 diagnostic, solvation-scheme follow-ups and the reaction-E0 check.Setting the booleans directly would convert a fail-safe bug, where good results are discarded, into a fail-open one where a stable species carrying an imaginary frequency is accepted as converged.
So piped tasks are routed through the same Scheduler methods the individually submitted ones use. This is cheap:
post_sp_actions()is already path-based and needs no adaptation, andcheck_negative_freq()reads onlylocal_path_to_output_filefrom its job argument, so a minimalPipedJobViewsupplies it.Finalization runs per task inside the ingestion loop rather than in
_post_ingest_pipe_run(), because a cross-species batch carries one species per task and because_post_ingest_pipe_run()is skipped entirely when any sibling task is ejected for troubleshooting. A task can reachCOMPLETEDwhile still holding a non-converged ESS output, so the convergence gate is repeated before any flag can be set.Why this went unnoticed
The path is only reached when a
species_sp/species_freqbatch actually forms, which needs at leastpipe_settings['min_tasks'](default 10) homogeneous species-level tasks in a single flush. Ordinary runs have far fewer species, fall through torun_sp_job()/run_freq_job()per label, and are unaffected. Existing production runs only ever formedts_optbatches, which have their own post-ingestion handler and never consultjob_types.It becomes reachable in large campaigns — which is exactly what pipe mode is for.
Nothing here is engine-specific; the ingestion helpers contain no engine branching.
Verification
End-to-end A/B on four species (CH4, OH, CH3, H2O), identical input, toggling only
pipe_settings['enabled'], withmin_tasks: 2so the batches actually form:Confirmed in the log that the batches genuinely formed (
Routing 4 species SP jobs to pipe mode), not that pipe mode was skipped.Tests: 6 new for the coordinator (the two positive ones fail without the fix; guard cases cover non-converged ESS, missing output, unknown species, and other families) and 1 for
has_pending_pipe_work()covering all five branches.arc/job/pipe/149 passed;arc/scheduler_test.py30 passed serially.Note:
arc/scheduler_test.pyhas pre-existing order-dependent failures under xdist (TestSchedulershares class-level fixtures and project directories across its tests). Under CI's-n 6 --dist worksteal,mainalready failstest_initialize_output_dictandtest_check_rxn_e0_by_spcwithout any of these changes; all pass with--dist loadscopeor-n 0. The new test is its ownTestCaseto avoid perturbing that class further._check_ess_convergenceis renamed tocheck_ess_convergencenow that it has a caller in a sibling module.