Skip to content

Let a pinned reaction family determine which families are loaded - #1038

Open
calvinp0 wants to merge 1 commit into
mainfrom
fix_pinned_family_narrows_load_set
Open

Let a pinned reaction family determine which families are loaded#1038
calvinp0 wants to merge 1 commit into
mainfrom
fix_pinned_family_narrows_load_set

Conversation

@calvinp0

@calvinp0 calvinp0 commented Sep 1, 2026

Copy link
Copy Markdown
Member

A reaction family pinned in input.yml now determines which families are loaded, instead of merely
filtering among the families the global setting happened to load.

The problem

ARCReaction.from_dict reads a pinned family and the setter records it, but the product dicts come
from the product_dicts property, which calls get_product_dicts() with rmg_family_set=None
and that falls back to settings['rmg_family_set']. So a pin could only ever select among families
that were already loaded.

XY_Addition_MultipleBond is the case that exposes it. Its group definition requires a halogen at
*4, so RMG files it under the halogens set rather than default. With ARC's shipped
rmg_family_set = 'default' the family is never loaded, and pinning it yields an empty
product_dicts — the user named a family and got nothing back.

There were two distinct failures, on two paths:

  • ARCReaction(family='XY_Addition_MultipleBond') raised ValueError: Invalid family name, because
    __init__'s check_family_name() validates against the configured set.
  • The input.yml route (arc/main.pyreaction_dictfrom_dict → the setter) fell silent,
    because the setter does not validate.

The change

When a family is pinned and no set is named explicitly, get_product_dicts narrows the load set to
that family. get_all_families already returns a list of family labels unchanged when it contains no
set names, so this needs no new lookup path.

determine_family's non-cached branch now routes through get_product_dicts rather than calling
get_reaction_family_products directly, which also removes a duplicated call.

ARC-native families need no special case. get_all_families(['ether_hydrolysis']) returns the
label as-is and read_groups_file_lines finds it under data/families/, so a pinned
carbonyl_based_hydrolysis, ether_hydrolysis or nitrile_hydrolysis resolves by the same route.

Reverse discovery is unaffected. get_reaction_family_products tries both directions for every
family regardless of how many are in the list, and family_own_reverse reads groups.py directly,
so it already worked for an out-of-set pin.

Loading one family rather than all of them takes a cold get_product_dicts from 0.28 s to 0.03 s.

A pin that cannot be honoured is an error

If the pinned family is available but does not match the reaction, get_product_dicts raises
ReactionError naming the families that do match, found by re-scanning under 'all':

Reaction <label> was assigned the <family> family, but it does not match this family.
The families it does match are: [...].

with a distinct message when nothing matches at all. A pin is a statement of intent, so failing to
honour it should stop the run rather than quietly produce a reaction with no family — and the message
carries what the next run should say instead.

A pinned family that is not a real family at all is caught earlier: check_family() runs from
check_attributes() at Scheduler setup, before any job is submitted.

Reuse

Searched before writing: every reader of settings['rmg_family_set'], every .product_dicts and
.determine_family( caller, the consider_*_families=False call sites, and an order-preserving
unique helper (dedup, uniq, dict.fromkeys). get_families_from_product_dicts() in
arc/family/family.py consolidates a loop that restrict_product_dicts_to_family had open-coded;
check_family_name gains an rmg_family_set argument rather than growing a second copy, and
is_family_available() is new.

rmg_family_set itself is unchanged and still ships as 'default'.

Tests

163 pass across arc/reaction/ and arc/family/. Full CI-style run,
pytest arc/ -n 6 --dist worksteal: 3173 passed, 43 skipped, 7 failed — all seven pre-existing, six
of them reproduced on an untouched main worktree (common_test::test_which, five torch_ani_test
cases) and the seventh a known worksteal ordering race that passes serially.

Validated by mutation rather than coverage — nine mutants, each killed by a test: the pin stops
narrowing; the gate keyed back on _family rather than the pin provenance; the availability check
dropped; the empty-result raise dropped; the diagnostic re-scan reverted to the configured set;
is_family_available losing either of its two terms; determine_family reverted to the direct call;
and check_family_name ignoring its new argument.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MbGeU8wLpafo3YFzTky2ho

Copilot AI lite review requested due to automatic review settings September 1, 2026 12:43

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.

Comment thread arc/reaction/reaction.py Dismissed
Comment thread arc/reaction/reaction.py Dismissed
Comment thread arc/reaction/reaction.py Dismissed
Comment thread arc/reaction/reaction.py Dismissed
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.53%. Comparing base (2aad076) to head (472017e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
- Coverage   65.55%   65.53%   -0.02%     
==========================================
  Files         120      120              
  Lines       40516    40539      +23     
  Branches    10441    10446       +5     
==========================================
+ Hits        26559    26568       +9     
- Misses      10952    10965      +13     
- Partials     3005     3006       +1     
Flag Coverage Δ
functionaltests 65.53% <ø> (-0.02%) ⬇️
unittests 65.53% <ø> (-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.

A family pinned on an ARCReaction only filtered among the families that
settings['rmg_family_set'] had already loaded. Pinning a family the
configured set does not list therefore produced no product dicts and no
explanation: no atom label map, no recipe, no family-derived reactive
bonds. XY_Addition_MultipleBond is the motivating case - it requires a
halogen at *4, so RMG lists it in the halogens set and ARC's shipped
'default' set does not reach it.

A pinned family is now the family set that get_product_dicts() loads,
which get_all_families() already supports by returning a list of labels
as given. RMG families and ARC's own families are both reached this way.
determine_family() routes its non-cached branch through
get_product_dicts() so the narrowing applies there too, which also
removes a duplicated call into the family module.

A family the family property determined lazily is not a pinned family
and does not narrow anything: the setter records the pin, and lazy
determination writes the private attribute directly.

A pin that cannot be honoured is now reported instead of yielding
nothing. An unavailable family name is rejected by check_family(), which
check_attributes() runs at reaction setup, before any job is scheduled. A
pin the reaction does not match raises and names the families it does
match, looked up across the available families rather than the
configured set, so a directory-only family such as H2_Loss is named.

check_family_name() takes the family set to look the family up in, and
is_family_available() asks whether a family can be loaded by label at
all. That union keeps a surface family pinnable, since 'all' skips the
surface sets and directories.

get_families_from_product_dicts() replaces the dedup loop that
restrict_product_dicts_to_family() open-coded.
@calvinp0
calvinp0 force-pushed the fix_pinned_family_narrows_load_set branch from 7b42704 to 472017e Compare September 1, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants