Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds GoFlow as a TS-guess adapter in ARC, with an installer, settings discovery, and CI coverage for a CPU-only GoFlow lane.
Changes:
- Added GoFlow TS adapter + subprocess-based inference script (keeps heavy ML deps out of ARC’s main env)
- Added GoFlow installer + wiring into
install_all.sh,Makefile, and a dedicated CI job - Added settings finders + Tier-1/Tier-2 unit tests for settings, adapter helpers, and end-to-end execution
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| devtools/install_goflow.sh | New installer for cloning goflow_lean, creating goflow_env, and acquiring/validating artifacts |
| devtools/install_all.sh | Adds optional GoFlow install step and --no-goflow flag + flag whitelist entry |
| arc/settings/settings_test.py | Unit tests for GoFlow settings finders and exported globals |
| arc/settings/settings.py | Adds GoFlow to settings globals and adds helper functions to find repo/ckpt/feat_dict |
| arc/main_test.py | Updates expected serialized ts_adapters list to include goflow |
| arc/job/adapters/ts/goflow_ts.py | New JobAdapter implementation for GoFlow TS guesses |
| arc/job/adapters/ts/goflow_test.py | Tier-1 helper tests + Tier-2 env-gated end-to-end tests |
| arc/job/adapters/ts/init.py | Imports GoFlow adapter module so it registers |
| arc/job/adapters/scripts/goflow_script_test.py | Tests for stdlib-only helper functions in the GoFlow script |
| arc/job/adapters/scripts/goflow_script.py | New standalone GoFlow inference runner executed inside goflow_env |
| arc/job/adapters/common.py | Includes goflow in default_incore_adapters |
| arc/job/adapter.py | Adds JobEnum.goflow |
| Makefile | Adds install-goflow target and excludes GoFlow from install-ci |
| .github/workflows/ci.yml | Adds a dedicated GoFlow install + test job (CPU lane) with caching |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #887 +/- ##
==========================================
+ Coverage 63.43% 63.61% +0.18%
==========================================
Files 114 117 +3
Lines 38325 38843 +518
Branches 10030 10123 +93
==========================================
+ Hits 24312 24711 +399
- Misses 11096 11191 +95
- Partials 2917 2941 +24
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:
|
b588a29 to
4f3407d
Compare
alongd
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (1)
arc/settings/settings.py:198
output_filenameswas updated forrits, but there is still no entry forgoflow.JobAdapterfalls back to'output.out'when a job_adapter key is missing, which can break queue/pipe bookkeeping for the GoFlow adapter (it actually produces/consumesoutput.yml). Consider adding agoflow: 'output.yml'mapping here for consistency with the adapter’s staged files.
output_filenames = {'cfour': 'output.out',
'gaussian': 'input.log',
'gcn': 'output.yml',
'mockter': 'output.yml',
'molpro': 'input.out',
'onedmin': 'output.out',
'orca': 'input.log',
'orca_neb': 'input.log',
'qchem': 'output.out',
'rits': 'output.yml',
'terachem': 'output.out',
'torchani': 'output.yml',
'xtb': 'output.out',
'openbabel':'output.yml',
}
| # TS methods to try when appropriate for a reaction (other than user guesses which are always allowed): | ||
| ts_adapters = ['heuristics', 'AutoTST', 'GCN', 'xtb_gsm', 'orca_neb'] | ||
| # Note: 'RitS' is intentionally NOT in the default — its env (rits_env + | ||
| # pretrained ckpt) is heavyweight, so users opt in explicitly via | ||
| # ``ts_adapters: ['rits', ...]`` in their input.yml. | ||
| ts_adapters = ['heuristics', 'AutoTST', 'goflow', 'orca_neb'] | ||
|
|
| GoFlow is **opt-in only** — its inference stack is heavyweight and the model is RDB7-trained, | ||
| so it is intentionally absent from the default ``ts_adapters`` list. The adapter enforces a | ||
| runtime domain guard: |
| def test_ts_adapters_includes_goflow_by_default(self): | ||
| """GoFlow is enabled by default — case-insensitive match (the default | ||
| list uses 'goflow' lowercase).""" | ||
| self.assertIn('goflow', [a.lower() for a in settings_mod.ts_adapters]) | ||
|
|
||
| def test_ts_adapters_does_not_include_rits_by_default(self): | ||
| """RitS's env (rits_env + pretrained ckpt) is heavyweight, so it must | ||
| stay opt-in. Users enable it via ``ts_adapters: ['rits', ...]`` in | ||
| their input.yml.""" | ||
| self.assertNotIn('RitS', settings_mod.ts_adapters) | ||
| self.assertNotIn('rits', settings_mod.ts_adapters) |
| job_types=self.job_types1, | ||
| species=[spc1], | ||
| level_of_theory='ccsd(t)-f12/cc-pvdz-f12//b3lyp/6-311+g(3df,2p)', | ||
| ts_adapters=['heuristics', 'AutoTST', 'GCN', 'xtb_gsm'], | ||
| ts_adapters=['heuristics', 'AutoTST', 'GCN', 'xtb_gsm', 'goflow'], | ||
| ) |
| VER=$(nvcc --version | grep -oP "release \K[0-9]+\.[0-9]+" | head -n1) | ||
| CUDA_VARIANT=$(map_cuda_to_variant "$VER") | ||
| echo "🔍 nvcc reports CUDA $VER → using PyG variant '$CUDA_VARIANT'" | ||
| elif command -v nvidia-smi &>/dev/null; then | ||
| # The 'CUDA Version' field in nvidia-smi is the *driver's max supported* CUDA, which is the | ||
| # right ceiling for binary wheel compatibility (not driver_version, which is a different number). | ||
| VER=$(nvidia-smi 2>/dev/null | grep -oP "CUDA Version: \K[0-9]+\.[0-9]+" | head -n1 || true) |
| HAS_RITS = _rits_environment_ready() | ||
|
|
||
|
|
| def test_returns_none_when_no_repo_and_no_env_var(self): | ||
| os.environ.pop('ARC_GOFLOW_CKPT', None) | ||
| self.assertIsNone(external_paths.find_goflow_ckpt(repo_path=None)) | ||
|
|
| def test_returns_none_when_no_repo_and_no_env_var(self): | ||
| os.environ.pop('ARC_GOFLOW_FEAT_DICT', None) | ||
| self.assertIsNone(external_paths.find_goflow_feat_dict(repo_path=None)) | ||
|
|
There was a problem hiding this comment.
Can you look into that? that sounds like it may endanger testers' env.
There was a problem hiding this comment.
I see many similar issues- I think they may be important.
There was a problem hiding this comment.
The copilot comments are outdated and were addressed. It should be already fixed (I just didn't mark the copilot comments are resolved)
There was a problem hiding this comment.
Already handled — every os.environ.pop in external_paths_test.py is now inside a mock.patch.dict(os.environ, ...) block, which restores the full dict on exit (12 call sites; none of the other new test files touch os.environ).
Worth noting the blast radius was narrower than it reads: os.environ is the pytest process's own copy, so a tester's shell was never at risk — the real exposure was cross-test leakage within a run, which matters under -n 6.
Happy to go through the other Copilot comments if you can point at which ones you meant.
| def test_returns_none_when_no_repo_and_no_env_var(self): | ||
| os.environ.pop('ARC_RITS_CKPT', None) | ||
| self.assertIsNone(external_paths.find_rits_ckpt(repo_path=None)) | ||
|
|
| } | ||
|
|
||
| all_families_ts_adapters = [] | ||
| all_families_ts_adapters = ['rits'] | ||
| adapters_that_do_not_require_a_level_arg = ['xtb', 'torchani'] | ||
|
|
||
| # Default is "queue", "pipe" will be called whenever needed. So just list 'incore'. | ||
| default_incore_adapters = ['autotst', 'crest', 'gcn', 'heuristics', 'kinbot', 'openbabel', 'torchani', 'psi4', 'xtb', 'xtb_gsm'] | ||
| default_incore_adapters = ['autotst', 'crest', 'gcn', 'goflow', 'heuristics', 'kinbot', 'openbabel', 'psi4', | ||
| 'rits', 'torchani', 'xtb', 'xtb_gsm'] |
| from collections import Counter | ||
| from unittest import mock | ||
|
|
||
| import arc.job.adapters.ts.rits_ts as rits_mod |
| --no-ext) SKIP_EXT=true ;; | ||
| --no-arc) SKIP_ARC=true ;; | ||
| --no-goflow) SKIP_GOFLOW=true ;; | ||
| --no-rits) SKIP_RITS=true ;; |
There was a problem hiding this comment.
Should we really add flag/software here? what about the other software?
There was a problem hiding this comment.
These envs are large, so we don't install them regularly in CI using these flags, at least indirectly (through install-ci). Then CI installs them in separate cached steps
| @@ -0,0 +1,47 @@ | |||
| #!/usr/bin/env python3 | |||
| # encoding: utf-8 | |||
There was a problem hiding this comment.
Great! We can probably expand this to other adapters. (out of scope for this PR, can you open an issue about that?)
| } | ||
|
|
||
| all_families_ts_adapters = [] | ||
| all_families_ts_adapters = ['goflow', 'rits'] |
There was a problem hiding this comment.
Yes, I tried CH4 + OH <=> CH3 + H2O for example, it gives results with correct connectivity (GoFlow seems better). So bimolecular works, but we;ll assess after the ARC benchmark and decide.
| def test_returns_none_when_no_repo_and_no_env_var(self): | ||
| os.environ.pop('ARC_GOFLOW_FEAT_DICT', None) | ||
| self.assertIsNone(external_paths.find_goflow_feat_dict(repo_path=None)) | ||
|
|
There was a problem hiding this comment.
Can you look into that? that sounds like it may endanger testers' env.
| def test_returns_none_when_no_repo_and_no_env_var(self): | ||
| os.environ.pop('ARC_GOFLOW_FEAT_DICT', None) | ||
| self.assertIsNone(external_paths.find_goflow_feat_dict(repo_path=None)) | ||
|
|
There was a problem hiding this comment.
I see many similar issues- I think they may be important.
| return 'cuda' if torch.cuda.is_available() else 'cpu' | ||
|
|
||
|
|
||
| def _validate_ckpt(ckpt_path: str) -> None: |
There was a problem hiding this comment.
Does that occure per compute? isn't most of it duplicated later in run_goflow_inference? I think we can safley make this function handle the validation and return the loaded model, and not loading it twice with torch.load.
There was a problem hiding this comment.
you're right, yes - it's per reaction. I'll fix
| """ | ||
| t0 = datetime.datetime.now() | ||
| try: | ||
| # Late imports: this function only runs inside goflow_env. |
There was a problem hiding this comment.
I thought the entire script runs in the goflow env
| # scope so that the module stays importable from ARC's main env, where torch/hydra/ | ||
| # goflow are absent — arc/job/adapters/scripts/goflow_script_test.py imports it there | ||
| # to unit-test the pure-Python helpers above. | ||
| import torch |
There was a problem hiding this comment.
As a convention, we import at the beginning of the script, unless there is a reason not to.
| # TS methods to try when appropriate for a reaction (other than user guesses which are always allowed): | ||
| ts_adapters = ['heuristics', 'AutoTST', 'GCN', 'xtb_gsm', 'orca_neb'] | ||
| # Note: 'RitS' is intentionally NOT in the default — its env (rits_env + | ||
| # pretrained ckpt) is heavyweight, so users opt in explicitly via | ||
| # ``ts_adapters: ['rits', ...]`` in their input.yml. | ||
| ts_adapters = ['heuristics', 'AutoTST', 'goflow', 'orca_neb'] | ||
|
|
Summary
Adds GoFlow and RitS, two flow-matching ML TS-guess adapters, sharing
the same lifecycle, the same Tier-1 / Tier-2 test split, and the same
"warn + skip" behavior when their respective env (
goflow_env/rits_env)or pretrained checkpoint is unavailable.
geometries from atom-mapped reactant + product SMILES. RDB7-trained, so it
enforces a runtime domain guard (
{H, C, N, O, F}, ≤100 atoms) and isnot added to
all_families_ts_adapters. Now part of the defaultts_adapterslist (replacesGCNandxtb_gsmin the default).atom-mapped reactant + product 3D structures. Handles bimolecular
reactions and charged species, so it is added to
all_families_ts_adapters. Heavyweight env, kept opt-in only viats_adapters: ['rits', ...].Sister-repo path discovery is consolidated into a single
arc/settings/external_paths.py(with matching test module). Both adaptersare inlined into the main CI lane (cached env + checkpoint per adapter), so
their Tier-2 end-to-end tests actually run on every PR.
Sources
GoFlow
epoch_316.ckpt, SHA-256-verified byinstall_goflow.sh.RitS
rits.ckpt, SHA-2562c6fdacc13a304cb8bb4030881ff4198de647d8f8f7fdaa878414e2514be0668.install_rits.shdownloads and verifies it automatically.