Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds contributor docs, PR template and Code of Conduct; introduces multi-OS CI, CodeQL scanning, and docs build; adds ReadTheDocs/Sphinx config and many docs pages; provides devtools Conda env and env-creation script; updates packaging/manifest and README; and replaces datasets main with a CLI (create/run/progress/analyze) including an analyze() helper. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Dataset as SinglepointDataset
participant FS as FileSystem
rect rgba(135,206,250,0.5)
User->>CLI: run analyze --input <ds_pickle> --output out.pkl
CLI->>Dataset: load from <ds_pickle>
CLI->>Dataset: iterate records with status="complete"
Dataset-->>CLI: record.dict()["properties"]
CLI->>FS: assemble DataFrame and write out.pkl (to_pickle)
FS-->>User: out.pkl saved
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (9)
.github/workflows/test.yml (1)
35-38: Consider consolidating documentation dependencies.The Sphinx dependencies are installed inline here, but docs/requirements.yaml also exists for Read the Docs. Consider reusing that file (
pip install -r docs/requirements.txtor conda environment) to ensure consistency and avoid maintaining dependencies in multiple places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 35 - 38, Replace the inline pip installs in the GitHub Actions step named "Install documentation dependencies" with a single command that installs from the shared docs requirements file (e.g., pip install -r docs/requirements.txt) or activate the conda environment generated from docs/requirements.yaml, so the workflow reuses docs/requirements.txt (or the conda env) and avoids duplicating Sphinx dependency definitions.docs/make.bat (1)
7-9: Quote SET values for safer batch scripting.Unquoted SET statements can cause issues with trailing spaces or special characters.
Proposed fix
if "%SPHINXBUILD%" == "" ( - set SPHINXBUILD=sphinx-build + set "SPHINXBUILD=sphinx-build" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/make.bat` around lines 7 - 9, Wrap the SET assignment value in quotes to avoid issues with trailing spaces or special characters: change the naked SET usage for the SPHINXBUILD variable (the line using set SPHINXBUILD=sphinx-build) to use quoted assignment (set "SPHINXBUILD=sphinx-build") so the variable and value are handled safely while leaving the existing if "%SPHINXBUILD%" == "" check intact.docs/README.md (1)
4-6: Missing list of dependencies to install.The instructions mention ensuring dependencies are installed but don't specify what they are. Consider adding the installation command:
To compile the docs, first ensure that the necessary dependencies are installed. - +```bash +pip install sphinx sphinx-rtd-theme +``` Once installed, you can use the `Makefile` in this directory to compile static HTML pages by🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` around lines 4 - 6, The README currently tells users to ensure dependencies are installed but doesn't list them; update docs/README.md to explicitly list the required Python packages (e.g., sphinx and sphinx-rtd-theme) and add the pip installation command to the dependencies section, then mention using the existing Makefile in this directory to build the HTML output (i.e., call the Makefile target for html). Ensure the added text references the packages by name and the Makefile usage so readers know exactly what to install and how to compile the docs.devtools/conda-envs/test_env.yaml (1)
24-27: Inconsistent version pinning style in conda dependencies.Lines 24-27 use
==(pip-style) instead of=(conda-style) for version pinning in the conda dependencies section. While conda often tolerates this, using=is more idiomatic.- - matplotlib==3.10.0 - - pydantic==2.* - - scipy==1.15.* - - tqdm==4.* + - matplotlib=3.10.0 + - pydantic=2.* + - scipy=1.15.* + - tqdm=4.*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devtools/conda-envs/test_env.yaml` around lines 24 - 27, Replace pip-style version pins using '==' with conda-style '=' for the listed dependencies so the conda env uses idiomatic pinning; specifically update the entries for matplotlib==3.10.0, pydantic==2.*, scipy==1.15.*, and tqdm==4.* to matplotlib=3.10.0, pydantic=2.*, scipy=1.15.*, and tqdm=4.* in the conda dependencies block.MANIFEST.in (1)
1-3: Consider explicitly includingdata/andmodels/in MANIFEST.in for clarity.The codebase loads package data at runtime using
importlib.resources:
src/apnet_pt/constants.pyloadsdata/vdw-params.csvat module importsrc/apnet_pt/pretrained_models.pyloads model files frommodels/While
setup.cfgalready includes these aspackage_data, the currentMANIFEST.inomits them. However, withinclude_package_data = Truein setup.cfg, both wheels and source distributions will include these directories via VCS tracking (git), so sdist installation will not break. Still, explicitly adding them improves clarity and robustness across different setuptools configurations.Optional addition to MANIFEST.in
include CODE_OF_CONDUCT.md +recursive-include src/apnet_pt/data * +recursive-include src/apnet_pt/models * global-exclude *.py[cod] __pycache__ *.so🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MANIFEST.in` around lines 1 - 3, MANIFEST.in currently omits the package data directories required at runtime; explicitly add the package data entries so sdist/wheel creators always include them: add include lines for the data and models directories (used by src/apnet_pt/constants.py which loads data/vdw-params.csv via importlib.resources and src/apnet_pt/pretrained_models.py which loads files from models/) even though setup.cfg has package_data and include_package_data; this clarifies intent and ensures distributions include those assets across different setuptools setups.docs/requirements.yaml (1)
9-27: Deduplicate the Conda dependency list.
pip,numpy,pandas, andpytorchare each declared twice here. Keeping one entry per package will make the docs environment easier to maintain and avoid silent pin drift later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/requirements.yaml` around lines 9 - 27, Duplicate package entries (pip, numpy, pandas, pytorch) appear in the Conda requirements list; remove the redundant lines so each package is declared exactly once, keeping the intended version pins (e.g., numpy=2.2.*, pandas=2.2.*, pytorch=2.7.0) and a single pip entry, and ensure other unique packages (psycopg2, pytest, flask, jinja2, sympy, sphinx deps) remain unchanged; update the requirements.yaml entries for these package names to be unique to avoid duplication and potential pin drift.devtools/scripts/create_conda_env.py (2)
28-32: Chain the exception for better traceability.When re-raising an exception within an
exceptblock, usefromto chain it. This preserves the original traceback context.♻️ Proposed fix
- except (KeyError, ImportError, IndexError): - raise ImportError("No YAML parser could be found in this or the conda environment. " + except (KeyError, ImportError, IndexError) as err: + raise ImportError("No YAML parser could be found in this or the conda environment. " "Could not find PyYAML or Ruamel YAML in the current environment, " - "AND could not find Ruamel YAML in the base conda environment through CONDA_EXE path. " - "Environment not created!") + "AND could not find Ruamel YAML in the base conda environment through CONDA_EXE path. " + "Environment not created!") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devtools/scripts/create_conda_env.py` around lines 28 - 32, In the except block catching (KeyError, ImportError, IndexError) replace the bare re-raise with an exception chain by capturing the original exception (e.g. except (KeyError, ImportError, IndexError) as e:) and re-raising the ImportError with "from e" so the original traceback is preserved when raising the ImportError("No YAML parser... Environment not created!") from e; keep the existing message text unchanged.
26-27:load_module()is deprecated.
spec.loader.load_module()has been deprecated since Python 3.4. Useimportlib.util.module_from_spec()andloader.exec_module()instead.♻️ Proposed fix using modern importlib API
spec = import_util.spec_from_file_location('ruamel_yaml', ruamel_yaml_path) - yaml = spec.loader.load_module() + yaml = import_util.module_from_spec(spec) + spec.loader.exec_module(yaml)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devtools/scripts/create_conda_env.py` around lines 26 - 27, The code uses the deprecated spec.loader.load_module() to load the ruamel YAML module; replace that with the modern importlib API by creating a module object with importlib.util.module_from_spec(spec) and then executing the loader via spec.loader.exec_module(module), assigning the resulting module to the yaml variable (replace usages of spec.loader.load_module() and the yaml assignment), while keeping the existing spec and ruamel_yaml_path logic intact.docs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rst (1)
9-16: Avoid documenting__init__twice.
__init__is included both via.. automethod:: __init__and again in the autosummary list, so the generated page is likely to show the constructor twice. Keeping only one source of truth will make the page cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rst` around lines 9 - 16, The RST currently documents AtomHirshfeldModel.__init__ twice (once with ".. automethod:: __init__" and again in the ".. autosummary::" block), causing duplicate constructor entries; remove one occurrence so the constructor is documented only once — either delete the standalone ".. automethod:: __init__" line or remove "~AtomHirshfeldModel.__init__" from the autosummary list in AtomHirshfeldModel.rst to keep a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/CONTRIBUTING.md:
- Around line 3-4: Replace the awkward phrase "merge code changes into this
qcmlforge" with clearer wording; update the sentence in CONTRIBUTING.md to read
either "merge code changes into qcmlforge" or "merge code changes into this
project" (or another concise alternative) so the sentence becomes: "We welcome
contributions from external contributors, and this document describes how to
merge code changes into qcmlforge." Ensure you only change that phrase and keep
the rest of the sentence intact.
In `@datasets/compute.py`:
- Around line 135-155: main() currently contains only its docstring but is still
invoked by the script entrypoint; restore the original mode-dispatch logic (or
remove the __main__ invocation) so the documented "create", "run", "progress",
and "analyze" modes actually execute. Reintroduce the switch on the mode
variable inside main() to call the existing workflow functions (e.g.,
create_dataset, submit_dataset, check_progress, analyze_results) or, if the
workflow was moved, update the __main__ guard to call the new entrypoint or
delete the invocation. Ensure the function name main() and mode handling are
consistent with other helper functions and that running the module no longer
silently exits without performing the described actions.
In `@devtools/conda-envs/test_env.yaml`:
- Around line 14-32: Remove the duplicate pytest entry in the conda environment
specification: keep the pinned version pytest=7.4.4 and delete the later
unpinned `pytest` entry so there is a single clear dependency declaration;
update the dependencies list in test_env.yaml by removing the bare "pytest" (the
second occurrence) to avoid ambiguity/conflict.
- Line 16: Replace the unavailable package spec 'pytorch=2.7.0' in the conda
environment definition with a valid version (for example 'pytorch=2.5.1') or
remove the pinned version to allow conda to resolve a compatible release; update
the 'pytorch=2.7.0' entry in the environment YAML (the conda package entry for
pytorch) so environment creation succeeds.
In `@devtools/README.md`:
- Line 29: The guidance sentence "Clone the repository if you have write access
to the main repo, fork the repository if you are a collaborator." is reversed;
update that line in devtools/README.md so it instructs users without write
access to fork and contributors/collaborators with write access to clone and
work on branches in the main repo—replace the sentence with something like:
"Fork the repository if you do not have write access to the main repo; if you
have write access, clone the main repository and work on a branch."
- Around line 47-56: Update the documented Versioneer regular expression in the
README where the regex block shows `\d+.\d+.\d+(?\+\d+-[a-z0-9]+)` so it becomes
a valid, precise pattern: escape the dots to match literal periods and replace
the malformed group with a non-capturing optional group (e.g.,
`\d+\.\d+\.\d+(?:\+\d+-[a-z0-9]+)?`) so the optional +commits-hash suffix is
correctly represented; update the code block containing the regex accordingly.
In `@devtools/scripts/create_conda_env.py`:
- Line 95: The subprocess invocation using sp.call with shell=True and a
formatted string is vulnerable to command injection; change the call in
create_conda_env.py (the sp.call(...) line) to use a list/sequence argument and
remove shell=True, e.g. build an argv list [conda_path, "env", "create", "-n",
args.name, "-f", temp_file_name] and pass that to sp.call (or sp.run) so
argument values are not interpreted by the shell; also validate/escape or
normalize args.name if needed and handle non-zero return codes or exceptions
from subprocess to surface failures.
- Line 94: The current call f.write(yaml.dump(yaml_script)) uses the
module-level dump which fails under ruamel.yaml; instead use the YAML instance
created earlier (the variable named yaml) to write to the open file stream: call
yaml.dump(yaml_script, f) (or yaml_instance.dump(yaml_script, f) if the instance
is named differently) rather than relying on a returned string, so replace
f.write(yaml.dump(yaml_script)) with a direct yaml.dump to the file stream using
the same safe YAML instance.
In `@docs/_static/README.md`:
- Around line 8-11: The README currently shows the wrong Sphinx setting: replace
the Jinja template override key templates_path with the static files key
html_static_path; update the example line that currently reads templates_path =
['_static'] to html_static_path = ['_static'] so the Sphinx conf uses the
correct symbol (html_static_path) for serving files from the _static folder.
In `@docs/_templates/README.md`:
- Around line 3-10: The README incorrectly mentions html_static_path for
template overrides; update the documentation to reference the Sphinx setting
templates_path instead (matching the repository's conf.py which uses
templates_path = ["_templates"]), i.e. replace any occurrence of
html_static_path = ['_templates'] with templates_path = ['_templates'] and
adjust the explanatory text to state that this controls HTML templates used to
override built-in templates (not static assets).
In `@docs/apnet_pt_models.rst`:
- Around line 124-126: The doc text wrongly calls the modules apnet3_fused and
apnet3_fused_variants “classes”; update the sentence to use the actual class
name APNet3_AtomType_Model instead of the module names so readers are directed
to the correct symbol—e.g., state that the modules expose the
APNet3_AtomType_Model class (with apnet3_fused being the main fused AP3
implementation and apnet3_fused_variants exposing the same class with larger
defaults and cutoffs).
In
`@docs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rst`:
- Line 1: Remove the UTF-8 BOM at the start of the docs page so the title line
"apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel" no longer
begins with a stray character; open the file
docs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rst
(look for the leading invisible BOM before the "apnet_pt..." title) and save it
without BOM (e.g., re-save as UTF-8 without BOM or strip the first byte) so the
AtomHirshfeldModel page title is clean.
In `@docs/autosummary/qcmlforge.qca.setup_qcarchive_qcfractal.rst`:
- Around line 1-2: Remove the leading BOM character and unescape the underscores
in the RST title "qcmlforge.qca.setup\_qcarchive\_qcfractal" (change to
"qcmlforge.qca.setup_qcarchive_qcfractal"), then adjust the equals-sign
underline so its length matches the title length (40 characters) exactly.
In `@docs/conf.py`:
- Around line 34-36: The docs are pulling version from the package attribute
qcmlforge.__version__ which currently yields 0.0.1; change docs/conf.py to
derive the version from the installed distribution metadata instead (e.g., use
importlib.metadata.version("qcmlforge") or importlib_metadata.version for older
Pythons) and assign that to the variables version and release (with a safe
fallback if the distribution is not found) so the Sphinx docs reflect the
pyproject.toml declared version; update the lines referencing version, release,
and qcmlforge accordingly.
- Around line 16-21: The import fails for uninstalled docs because
sys.path.insert currently adds the project root ("..") instead of the source
directory; update the sys.path insertion in conf.py (the sys.path.insert(0, ...)
call) to prepend the absolute path to the src directory (so Python can resolve
import qcmlforge from src/qcmlforge) before the import qcmlforge line.
In `@docs/getting_started.rst`:
- Around line 107-124: The docs show outdated CLI flags for train_models.py;
update the examples to use the current CLI options (referencing train_models.py
and the flags the script currently parses) — replace the nonexistent --train_ap2
with the correct invocation (e.g., include the actual train command or --train
flag used by train_models.py and the appropriate --model or --model_name value
for APNet2), and change the atomic multipole example to pass a model name to
--train_am (or to the current equivalent like --model_name <name>) instead of
using it as a bare flag so both examples match the script’s current argument
parsing.
In `@docs/make.bat`:
- Around line 1-36: The make.bat file uses LF line endings which can break batch
parsing on Windows; convert make.bat to use Windows CRLF line endings (ensure
ECHO OFF and labels like :help remain unchanged) and add a .gitattributes entry
to enforce CRLF for .bat files by adding the rule for *.bat text eol=crlf so Git
will auto-convert on checkout/commit.
In `@docs/user_guide.rst`:
- Around line 52-57: Update the documentation in train_models.py section to
replace the stale APNet flag reference `--train_ap2` with the current CLI flag
`--train_apnet` and clarify that it accepts the model name as its value; ensure
the paragraph listing examples now reads something like: AtomModel multipole
training with `--train_am` and APNet training with `--train_apnet <model_name>`
so readers are pointed to the actual option implemented in train_models.py.
In `@src/apnet_pt/atomic_datasets.py`:
- Around line 42-70: The docstring for natural_key currently contains an
accidental pasted helper (the generate_monomer_multipole_dataset snippet) which
keeps those lines inside the triple-quoted string and breaks the docstring and
function definition; fix this by closing the natural_key docstring before the
pasted lines and moving the generate_monomer_multipole_dataset definition
(including its body: calling util.load_monomer_dataset and returning the
appropriate value) out of the triple quotes so
generate_monomer_multipole_dataset is a real function, and ensure natural_key's
examples and closing triple quotes remain intact; reference natural_key and
generate_monomer_multipole_dataset to locate the places to edit.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 35-38: Replace the inline pip installs in the GitHub Actions step
named "Install documentation dependencies" with a single command that installs
from the shared docs requirements file (e.g., pip install -r
docs/requirements.txt) or activate the conda environment generated from
docs/requirements.yaml, so the workflow reuses docs/requirements.txt (or the
conda env) and avoids duplicating Sphinx dependency definitions.
In `@devtools/conda-envs/test_env.yaml`:
- Around line 24-27: Replace pip-style version pins using '==' with conda-style
'=' for the listed dependencies so the conda env uses idiomatic pinning;
specifically update the entries for matplotlib==3.10.0, pydantic==2.*,
scipy==1.15.*, and tqdm==4.* to matplotlib=3.10.0, pydantic=2.*, scipy=1.15.*,
and tqdm=4.* in the conda dependencies block.
In `@devtools/scripts/create_conda_env.py`:
- Around line 28-32: In the except block catching (KeyError, ImportError,
IndexError) replace the bare re-raise with an exception chain by capturing the
original exception (e.g. except (KeyError, ImportError, IndexError) as e:) and
re-raising the ImportError with "from e" so the original traceback is preserved
when raising the ImportError("No YAML parser... Environment not created!") from
e; keep the existing message text unchanged.
- Around line 26-27: The code uses the deprecated spec.loader.load_module() to
load the ruamel YAML module; replace that with the modern importlib API by
creating a module object with importlib.util.module_from_spec(spec) and then
executing the loader via spec.loader.exec_module(module), assigning the
resulting module to the yaml variable (replace usages of
spec.loader.load_module() and the yaml assignment), while keeping the existing
spec and ruamel_yaml_path logic intact.
In
`@docs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rst`:
- Around line 9-16: The RST currently documents AtomHirshfeldModel.__init__
twice (once with ".. automethod:: __init__" and again in the ".. autosummary::"
block), causing duplicate constructor entries; remove one occurrence so the
constructor is documented only once — either delete the standalone "..
automethod:: __init__" line or remove "~AtomHirshfeldModel.__init__" from the
autosummary list in AtomHirshfeldModel.rst to keep a single source of truth.
In `@docs/make.bat`:
- Around line 7-9: Wrap the SET assignment value in quotes to avoid issues with
trailing spaces or special characters: change the naked SET usage for the
SPHINXBUILD variable (the line using set SPHINXBUILD=sphinx-build) to use quoted
assignment (set "SPHINXBUILD=sphinx-build") so the variable and value are
handled safely while leaving the existing if "%SPHINXBUILD%" == "" check intact.
In `@docs/README.md`:
- Around line 4-6: The README currently tells users to ensure dependencies are
installed but doesn't list them; update docs/README.md to explicitly list the
required Python packages (e.g., sphinx and sphinx-rtd-theme) and add the pip
installation command to the dependencies section, then mention using the
existing Makefile in this directory to build the HTML output (i.e., call the
Makefile target for html). Ensure the added text references the packages by name
and the Makefile usage so readers know exactly what to install and how to
compile the docs.
In `@docs/requirements.yaml`:
- Around line 9-27: Duplicate package entries (pip, numpy, pandas, pytorch)
appear in the Conda requirements list; remove the redundant lines so each
package is declared exactly once, keeping the intended version pins (e.g.,
numpy=2.2.*, pandas=2.2.*, pytorch=2.7.0) and a single pip entry, and ensure
other unique packages (psycopg2, pytest, flask, jinja2, sympy, sphinx deps)
remain unchanged; update the requirements.yaml entries for these package names
to be unique to avoid duplication and potential pin drift.
In `@MANIFEST.in`:
- Around line 1-3: MANIFEST.in currently omits the package data directories
required at runtime; explicitly add the package data entries so sdist/wheel
creators always include them: add include lines for the data and models
directories (used by src/apnet_pt/constants.py which loads data/vdw-params.csv
via importlib.resources and src/apnet_pt/pretrained_models.py which loads files
from models/) even though setup.cfg has package_data and include_package_data;
this clarifies intent and ensures distributions include those assets across
different setuptools setups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 444c8d19-fbf9-4fab-bfef-f39a228e0cb4
📒 Files selected for processing (43)
.github/CONTRIBUTING.md.github/PULL_REQUEST_TEMPLATE.md.github/workflows/CI.yaml.github/workflows/codeql.yaml.github/workflows/test.yml.readthedocs.yamlCODE_OF_CONDUCT.mdMANIFEST.indatasets/compute.pydevtools/README.mddevtools/conda-envs/test_env.yamldevtools/scripts/create_conda_env.pydocs/Makefiledocs/README.mddocs/_static/README.mddocs/_templates/README.mddocs/api.rstdocs/apnet_pt_models.rstdocs/autosummary/apnet_pt.AtomModels.ap2_atom_model.AtomModel.rstdocs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rstdocs/autosummary/apnet_pt.AtomModels.ap3_atom_model.AtomInducedDipoleModel.rstdocs/autosummary/apnet_pt.AtomModels.ap3_atom_model_frozen.InducedDipoleModel.rstdocs/autosummary/apnet_pt.AtomModels.ap3_atomtype_mpnn.AtomTypeParamModel.rstdocs/autosummary/apnet_pt.AtomPairwiseModels.apnet2.APNet2Model.rstdocs/autosummary/apnet_pt.AtomPairwiseModels.apnet2_fused.APNet2_AM_Model.rstdocs/autosummary/apnet_pt.AtomPairwiseModels.apnet3_fused.APNet3_AtomType_Model.rstdocs/autosummary/apnet_pt.AtomPairwiseModels.apnet3_fused_variants.APNet3_AtomType_Model.rstdocs/autosummary/apnet_pt.AtomPairwiseModels.dapnet2.dAPNet2Model.rstdocs/autosummary/apnet_pt.AtomPairwiseModels.mtp_mtp.AM_DimerParam_Model.rstdocs/autosummary/apnet_pt.AtomPairwiseModels.mtp_mtp.AtomTypeParamModel.rstdocs/autosummary/apnet_pt.multipole.rstdocs/autosummary/apnet_pt.rstdocs/autosummary/qcmlforge.qca.setup_qcarchive_qcfractal.rstdocs/conf.pydocs/developer_guide.rstdocs/getting_started.rstdocs/index.rstdocs/make.batdocs/requirements.yamldocs/user_guide.rstenvironment.ymlsrc/apnet_pt/__init__.pysrc/apnet_pt/atomic_datasets.py
| @ECHO OFF | ||
|
|
||
| pushd %~dp0 | ||
|
|
||
| REM Command file for Sphinx documentation | ||
|
|
||
| if "%SPHINXBUILD%" == "" ( | ||
| set SPHINXBUILD=sphinx-build | ||
| ) | ||
| set SOURCEDIR=. | ||
| set BUILDDIR=_build | ||
| set SPHINXPROJ=qcmlforge | ||
|
|
||
| if "%1" == "" goto help | ||
|
|
||
| %SPHINXBUILD% >NUL 2>NUL | ||
| if errorlevel 9009 ( | ||
| echo. | ||
| echo.The 'sphinx-build' command was not found. Make sure you have Sphinx | ||
| echo.installed, then set the SPHINXBUILD environment variable to point | ||
| echo.to the full path of the 'sphinx-build' executable. Alternatively you | ||
| echo.may add the Sphinx directory to PATH. | ||
| echo. | ||
| echo.If you don't have Sphinx installed, grab it from | ||
| echo.http://sphinx-doc.org/ | ||
| exit /b 1 | ||
| ) | ||
|
|
||
| %SPHINXBUILD% -M %1 %SOURCEDIR% %BUILDDIR% %SPHINXOPTS% | ||
| goto end | ||
|
|
||
| :help | ||
| %SPHINXBUILD% -M help %SOURCEDIR% %BUILDDIR% %SPHINXOPTS% | ||
|
|
||
| :end | ||
| popd |
There was a problem hiding this comment.
Convert to Windows line endings (CRLF) for batch file compatibility.
This batch file uses Unix line endings (LF), which can cause parsing failures on Windows due to the batch parser's 512-byte boundary handling. The GOTO/CALL label parsing may fail intermittently.
Configure git to auto-convert line endings for .bat files by adding to .gitattributes:
*.bat text eol=crlf
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 36 LF sequences found
(E018)
[error] 8-8: Unsafe SET command usage. Explanation: SET commands without proper validation or quoting can cause security issues. Recommendation: Always quote SET values and validate input: SET "var=safe value". Context: SET command value should be quoted for safety
(SEC002)
[error] 16-16: Command injection via variable substitution. Explanation: Variables containing user input used in commands may allow code injection. Recommendation: Validate and sanitize variables before use in command execution. Context: Variable used with shell operators may allow injection
(SEC013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/make.bat` around lines 1 - 36, The make.bat file uses LF line endings
which can break batch parsing on Windows; convert make.bat to use Windows CRLF
line endings (ensure ECHO OFF and labels like :help remain unchanged) and add a
.gitattributes entry to enforce CRLF for .bat files by adding the rule for *.bat
text eol=crlf so Git will auto-convert on checkout/commit.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
21-33:⚠️ Potential issue | 🟡 MinorTroubleshooting text no longer matches the commands.
The section says the error is related to
torch-scatter, but the commands now only uninstall/reinstalltorch-geometric. Please align the wording so users apply the right fix.Suggested doc correction
-If you get an OS.Error when running qcml related to torch-scatter, you likely need -to install a specific version through the following example: +If you hit PyG-related import/runtime issues when running QCML, reinstall +`torch-geometric` for your Torch/CUDA build using the examples below:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 21 - 33, Update the README troubleshooting text so it accurately describes the package being reinstalled by the commands: either change the sentence that references "torch-scatter" to instead reference "torch-geometric (and its torch-scatter dependency)" or explicitly instruct users to reinstall torch-scatter as well; ensure the prose and the provided shell commands (which call pip uninstall/install for torch-geometric and reference TORCH/CUDA variables) consistently mention the same package(s) (torch-geometric and/or torch-scatter) and recommend the matching pip uninstall/install steps for the package named in the narrative.
🧹 Nitpick comments (2)
devtools/README.md (1)
23-23: Use hyphenated “OS-agnostic” wording.Small readability fix for user-facing docs.
Suggested wording tweak
-This directory contains OS agnostic helper scripts which don't fall in any of the previous categories +This directory contains OS-agnostic helper scripts which don't fall in any of the previous categories🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devtools/README.md` at line 23, Update the README sentence that currently reads "OS agnostic helper scripts" to use the hyphenated form "OS-agnostic helper scripts" for consistency and readability; locate the sentence in the README content string "This directory contains OS agnostic helper scripts which don't fall in any of the previous categories" and replace "OS agnostic" with "OS-agnostic".docs/_static/README.md (1)
5-6: Prefer “built-in” in prose.Tiny docs polish for consistency/readability.
Suggested wording tweak
-They are copied after the builtin static files, -so a file named "default.css" will overwrite the builtin "default.css". +They are copied after the built-in static files, +so a file named "default.css" will overwrite the built-in "default.css".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/_static/README.md` around lines 5 - 6, Replace occurrences of the word "builtin" with the hyphenated form "built-in" in the README sentence that currently reads "They are copied after the builtin static files, so a file named "default.css" will overwrite the builtin "default.css"." — update both instances so the prose uses "built-in static files" and "built-in "default.css"" to improve consistency and readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@datasets/compute.py`:
- Around line 217-225: The default CLI paths passed to parser.add_argument use
relative paths like "../data_dir/..." resolved from the current working
directory; change them to be repository-relative by constructing defaults from
the script location (use Path(__file__).resolve().parent) or require explicit
absolute paths as inputs. Update the two parser.add_argument calls that set
"--input" and "--output" so their default values are built from Path(__file__)
(or an explicit CLI parameter) instead of hard-coded "../..." strings, ensuring
create/analyze work regardless of the caller's cwd.
- Around line 267-269: The code assigns df via pd.read_pickle(args.input) when
args.mode == "create", which is unsafe for untrusted CLI input; replace this
with a safe deserialization path (e.g., pd.read_json(args.input),
pd.read_csv(args.input), or pd.read_parquet(args.input)) or add an explicit
trusted flag (e.g., --trusted-input) that must be set before allowing
pd.read_pickle. Update the branch that references df and args.mode to accept the
chosen safe loader and validate the file extension or mime-type before loading,
and only call pd.read_pickle(args.input) when the explicit trusted opt-in flag
is present.
- Around line 66-76: The code currently calls client.add_dataset()
unconditionally in add_QCdataset(), which can create empty datasets in
non-create modes and masks real errors with a broad except; change the control
flow so client.add_dataset(...) is invoked only when mode == "create" (keep the
existing df check in that branch), and for non-create modes use
client.get_dataset(...) (and raise a clear error if it doesn't exist). Replace
the blanket "except Exception" around client.add_dataset() with a narrow except
that catches the specific QCPortal duplicate-name exception (e.g., the
DuplicateDataset/duplicate-name error class your portal library raises) and
falls back to client.get_dataset() only in that case, while re-raising other
exceptions so timeouts/auth errors surface. Ensure references to
add_QCdataset(), client.add_dataset(), client.get_dataset(), mode, and df are
updated accordingly.
In `@devtools/scripts/create_conda_env.py`:
- Around line 72-77: Make the command-line flags mandatory by updating the
parser.add_argument calls for "-n/--name" and "-p/--python" to include
required=True, and add a short validation that args.name and args.python are
present before building the dependency string and invoking conda; specifically
update the parser.add_argument entries and ensure the code that constructs the
dependency string (where it currently produces "python None*") and the conda
command invocation use args.python and args.name only after confirming they are
non-null (raise a clear error or exit if missing).
- Around line 88-100: The code resets yaml_script["dependencies"] to [] because
re.match is called on non-string dep_value and the resulting TypeError is caught
by the broad except; change the logic to first ensure
yaml_script.get("dependencies") is a list and check each dep_value is a str
before calling re.match (e.g., skip non-str entries), and narrow the exception
handling to only catch KeyError (or validate types up-front) so dict entries
like pip: blocks are preserved; keep inserting python_replacement_string at
position 0 after safely removing any matching string entry from
yaml_script["dependencies"] (use yaml_script["dependencies"] as a list and break
after pop as currently done).
---
Outside diff comments:
In `@README.md`:
- Around line 21-33: Update the README troubleshooting text so it accurately
describes the package being reinstalled by the commands: either change the
sentence that references "torch-scatter" to instead reference "torch-geometric
(and its torch-scatter dependency)" or explicitly instruct users to reinstall
torch-scatter as well; ensure the prose and the provided shell commands (which
call pip uninstall/install for torch-geometric and reference TORCH/CUDA
variables) consistently mention the same package(s) (torch-geometric and/or
torch-scatter) and recommend the matching pip uninstall/install steps for the
package named in the narrative.
---
Nitpick comments:
In `@devtools/README.md`:
- Line 23: Update the README sentence that currently reads "OS agnostic helper
scripts" to use the hyphenated form "OS-agnostic helper scripts" for consistency
and readability; locate the sentence in the README content string "This
directory contains OS agnostic helper scripts which don't fall in any of the
previous categories" and replace "OS agnostic" with "OS-agnostic".
In `@docs/_static/README.md`:
- Around line 5-6: Replace occurrences of the word "builtin" with the hyphenated
form "built-in" in the README sentence that currently reads "They are copied
after the builtin static files, so a file named "default.css" will overwrite the
builtin "default.css"." — update both instances so the prose uses "built-in
static files" and "built-in "default.css"" to improve consistency and
readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31f9dba2-a7c4-4f85-9e60-f12780da8913
📒 Files selected for processing (24)
.gitattributes.github/CONTRIBUTING.md.github/workflows/test.ymlMANIFEST.inREADME.mdap3_testing_variant.ptdatasets/compute.pydevtools/README.mddevtools/conda-envs/test_env.yamldevtools/scripts/create_conda_env.pydocs/README.mddocs/_static/README.mddocs/_templates/README.mddocs/apnet_pt_models.rstdocs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rstdocs/autosummary/qcmlforge.qca.setup_qcarchive_qcfractal.rstdocs/conf.pydocs/getting_started.rstdocs/make.batdocs/requirements.txtdocs/requirements.yamldocs/user_guide.rstenvironment.ymlsrc/apnet_pt/atomic_datasets.py
✅ Files skipped from review due to trivial changes (13)
- .gitattributes
- docs/requirements.txt
- MANIFEST.in
- docs/_templates/README.md
- .github/CONTRIBUTING.md
- docs/README.md
- docs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rst
- devtools/conda-envs/test_env.yaml
- docs/apnet_pt_models.rst
- docs/requirements.yaml
- docs/autosummary/qcmlforge.qca.setup_qcarchive_qcfractal.rst
- docs/getting_started.rst
- docs/user_guide.rst
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/test.yml
- src/apnet_pt/atomic_datasets.py
- environment.yml
- docs/make.bat
| try: | ||
| ds = client.add_dataset("singlepoint", system, | ||
| f"Dataset to contain {system}") | ||
| ds = client.add_dataset("singlepoint", system, f"Dataset to contain {system}") | ||
| print(f"Added {system} as dataset") | ||
| except Exception: | ||
| ds = client.get_dataset("singlepoint", system) | ||
| print(f"Found {system} dataset, using this instead") | ||
| print(ds) | ||
|
|
||
| if mode == "create": | ||
| if df is None: | ||
| raise ValueError("A DataFrame is required when mode='create'.") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the datasets/compute.py file
find . -name "compute.py" -path "*/datasets/*" | head -20Repository: Awallace3/QCMLForge
Length of output: 84
🏁 Script executed:
# Get the file structure and size
wc -l datasets/compute.py 2>/dev/null || echo "File not found at expected path"Repository: Awallace3/QCMLForge
Length of output: 86
🏁 Script executed:
# Examine the relevant sections of the file
cat -n datasets/compute.py | sed -n '1,120p'Repository: Awallace3/QCMLForge
Length of output: 5263
🏁 Script executed:
# Look at the add_QCdataset function more broadly
cat -n datasets/compute.py | grep -A 50 "def add_QCdataset"Repository: Awallace3/QCMLForge
Length of output: 2406
🏁 Script executed:
# Look at main() and parse_args() to understand mode usage
cat -n datasets/compute.py | sed -n '207,289p'Repository: Awallace3/QCMLForge
Length of output: 3418
🏁 Script executed:
# Look for all mode values used
rg "mode\s*=\s*['\"]" datasets/compute.pyRepository: Awallace3/QCMLForge
Length of output: 192
🏁 Script executed:
# Check how add_QCdataset is called from main
rg "add_QCdataset" datasets/compute.py -A 3 -B 1Repository: Awallace3/QCMLForge
Length of output: 260
Move dataset creation into create mode only.
add_QCdataset() calls client.add_dataset() unconditionally at line 67, before checking mode at line 74. This means run, progress, and analyze modes will attempt to create a remote dataset on every invocation. A typo in --system during any non-create mode will silently create an empty dataset on the server. Additionally, the blanket except Exception mask all errors—network timeouts, authentication failures, and other portal issues—treating them all as "dataset already exists", making debugging harder. Restructure the control flow so only create mode calls add_dataset(), and replace the blanket exception handler with explicit handling for the duplicate-name error that QCPortal raises.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 69-69: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@datasets/compute.py` around lines 66 - 76, The code currently calls
client.add_dataset() unconditionally in add_QCdataset(), which can create empty
datasets in non-create modes and masks real errors with a broad except; change
the control flow so client.add_dataset(...) is invoked only when mode ==
"create" (keep the existing df check in that branch), and for non-create modes
use client.get_dataset(...) (and raise a clear error if it doesn't exist).
Replace the blanket "except Exception" around client.add_dataset() with a narrow
except that catches the specific QCPortal duplicate-name exception (e.g., the
DuplicateDataset/duplicate-name error class your portal library raises) and
falls back to client.get_dataset() only in that case, while re-raising other
exceptions so timeouts/auth errors surface. Ensure references to
add_QCdataset(), client.add_dataset(), client.get_dataset(), mode, and df are
updated accordingly.
| parser.add_argument( | ||
| "--input", | ||
| default="../data_dir/raw/monomers_ap3_spec_1.pkl", | ||
| help="Pickle file used to create the dataset.", | ||
| ) | ||
| parser.add_argument( | ||
| "--output", | ||
| default="../data_dir/raw/monomers_ap3_spec_1_pbe0.pkl", | ||
| help="Pickle file written during analyze mode.", |
There was a problem hiding this comment.
These defaults are tied to the caller’s working directory.
../data_dir/... is resolved from cwd, not from datasets/compute.py. Running the entrypoint from the repo root (python datasets/compute.py ...) points both defaults outside the repo, so create/analyze fail unless the user happens to launch from datasets/. Resolve them from Path(__file__) or make the paths explicit CLI inputs.
📁 Suggested fix
import os
import argparse
+from pathlib import Path
+
import pandas as pd
@@
+DATA_DIR = Path(__file__).resolve().parents[1] / "data_dir" / "raw"
+
def parse_args() -> argparse.Namespace:
"""Parse command-line arguments for dataset workflow management."""
@@
parser.add_argument(
"--input",
- default="../data_dir/raw/monomers_ap3_spec_1.pkl",
+ default=str(DATA_DIR / "monomers_ap3_spec_1.pkl"),
help="Pickle file used to create the dataset.",
)
parser.add_argument(
"--output",
- default="../data_dir/raw/monomers_ap3_spec_1_pbe0.pkl",
+ default=str(DATA_DIR / "monomers_ap3_spec_1_pbe0.pkl"),
help="Pickle file written during analyze mode.",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@datasets/compute.py` around lines 217 - 225, The default CLI paths passed to
parser.add_argument use relative paths like "../data_dir/..." resolved from the
current working directory; change them to be repository-relative by constructing
defaults from the script location (use Path(__file__).resolve().parent) or
require explicit absolute paths as inputs. Update the two parser.add_argument
calls that set "--input" and "--output" so their default values are built from
Path(__file__) (or an explicit CLI parameter) instead of hard-coded "../..."
strings, ensuring create/analyze work regardless of the caller's cwd.
| df = None | ||
| if args.mode == "create": | ||
| df = pd.read_pickle(args.input) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n datasets/compute.py | head -300 | tail -50Repository: Awallace3/QCMLForge
Length of output: 2027
🏁 Script executed:
grep -n "input\|argparse\|ArgumentParser" datasets/compute.py | head -20Repository: Awallace3/QCMLForge
Length of output: 233
🏁 Script executed:
web_search
Pickle deserialization arbitrary code execution vulnerabilityRepository: Awallace3/QCMLForge
Length of output: 158
🏁 Script executed:
sed -n '207,235p' datasets/compute.pyRepository: Awallace3/QCMLForge
Length of output: 1085
🌐 Web query:
pickle deserialization arbitrary code execution vulnerability Python
💡 Result:
Yes, Python's pickle module has a well-known arbitrary code execution vulnerability during deserialization of untrusted data. The official Python documentation explicitly warns: "The pickle module is not secure. Only unpickle data you trust. It is possible to construct malicious pickle data which will execute arbitrary code during unpickling. Never unpickle data that could have come from an untrusted source." This occurs because pickle can invoke arbitrary functions via the reduce method on custom classes in the pickled data, allowing attackers to execute system commands or other code upon calling pickle.loads or pickle.load. This is not a recent patchable bug in pickle itself but a fundamental design limitation present across all Python versions. Numerous exploits and CVEs in third-party libraries (e.g., python-socketio CVE-2025-61765, PLY CVE-2025-56005, llama_index CVE-2025-3108, picklescan CVE-2025-1716) demonstrate real-world impacts when untrusted data is deserialized. Mitigations: - Never deserialize untrusted pickle data. - Use safe alternatives like JSON (json module), MessagePack, or Protocol Buffers for untrusted inputs. - For trusted data only, consider restricted unpicklers (subclass Unpickler and override find_class). - Sign data with HMAC to detect tampering even for trusted sources. - Libraries like skops or SafeSerialize provide safer serialization for specific use cases like ML models. Example exploit payload: import pickle import os class RCE: def reduce(self): return (os.system, ('whoami',)) payload = pickle.dumps(RCE) pickle.loads(payload) # Executes 'whoami'
Citations:
- 1: https://docs.python.org/3.11/library/pickle.html
- 2: https://docs.python.org/3/library/pickle.html
- 3: https://deniskim1.com/writing/pickle_deserialization_attacks
- 4: https://davidhamann.de/2020/04/05/exploiting-python-pickle/
- 5: GHSA-g8c6-8fjj-2r4m
- 6: https://seclists.org/oss-sec/2026/q1/107
- 7: https://nvd.nist.gov/vuln/detail/CVE-2025-3108
Switch from pd.read_pickle() to a safer serialization format for --input.
pd.read_pickle(args.input) deserializes pickle data, which can execute arbitrary code. Since --input is caller-controlled via CLI, this creates an arbitrary-code-execution vector if someone provides a malicious pickle file. Use JSON, CSV, or Parquet instead, or require an explicit trusted-input opt-in if pickle is necessary.
🧰 Tools
🪛 Ruff (0.15.6)
[error] 269-269: pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue
(S301)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@datasets/compute.py` around lines 267 - 269, The code assigns df via
pd.read_pickle(args.input) when args.mode == "create", which is unsafe for
untrusted CLI input; replace this with a safe deserialization path (e.g.,
pd.read_json(args.input), pd.read_csv(args.input), or
pd.read_parquet(args.input)) or add an explicit trusted flag (e.g.,
--trusted-input) that must be set before allowing pd.read_pickle. Update the
branch that references df and args.mode to accept the chosen safe loader and
validate the file extension or mime-type before loading, and only call
pd.read_pickle(args.input) when the explicit trusted opt-in flag is present.
| parser.add_argument( | ||
| "-n", "--name", type=str, help="The name of the created Python environment" | ||
| ) | ||
| parser.add_argument( | ||
| "-p", "--python", type=str, help="The version of the created Python environment" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import ast
from pathlib import Path
src = Path("devtools/scripts/create_conda_env.py").read_text()
tree = ast.parse(src)
for node in ast.walk(tree):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr == "add_argument":
flags = [a.value for a in node.args if isinstance(a, ast.Constant) and isinstance(a.value, str)]
if "--name" in flags or "--python" in flags:
required_kw = next((k for k in node.keywords if k.arg == "required"), None)
required = isinstance(required_kw.value, ast.Constant) and required_kw.value.value is True if required_kw else False
print(flags, "required=", required)
PYRepository: Awallace3/QCMLForge
Length of output: 130
🏁 Script executed:
cat -n devtools/scripts/create_conda_env.pyRepository: Awallace3/QCMLForge
Length of output: 5318
Make --name and --python required.
Both flags are optional, allowing the script to proceed with None values. Line 86 creates an invalid dependency string "python None*", and line 123 passes None to the conda command, both causing runtime failures with unhelpful error messages.
Suggested fix
parser.add_argument(
- "-n", "--name", type=str, help="The name of the created Python environment"
+ "-n",
+ "--name",
+ type=str,
+ required=True,
+ help="The name of the created Python environment",
)
parser.add_argument(
- "-p", "--python", type=str, help="The version of the created Python environment"
+ "-p",
+ "--python",
+ type=str,
+ required=True,
+ help="The version of the created Python environment",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devtools/scripts/create_conda_env.py` around lines 72 - 77, Make the
command-line flags mandatory by updating the parser.add_argument calls for
"-n/--name" and "-p/--python" to include required=True, and add a short
validation that args.name and args.python are present before building the
dependency string and invoking conda; specifically update the
parser.add_argument entries and ensure the code that constructs the dependency
string (where it currently produces "python None*") and the conda command
invocation use args.python and args.name only after confirming they are non-null
(raise a clear error or exit if missing).
| try: | ||
| for dep_index, dep_value in enumerate(yaml_script["dependencies"]): | ||
| if re.match( | ||
| "python([ ><=*]+[0-9.*]*)?$", dep_value | ||
| ): # Match explicitly 'python' and its formats | ||
| yaml_script["dependencies"].pop(dep_index) | ||
| break # Making the assumption there is only one Python entry, also avoids need to enumerate in reverse | ||
| except (KeyError, TypeError): | ||
| # Case of no dependencies key, or dependencies: None | ||
| yaml_script["dependencies"] = [] | ||
| finally: | ||
| # Ensure the python version is added in. Even if the code does not need it, we assume the env does | ||
| yaml_script["dependencies"].insert(0, python_replacement_string) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import re
# Reproduce current behavior from the reviewed block:
yaml_script = {"dependencies": ["numpy", {"pip": ["foo==1.0"]}]}
python_replacement_string = "python 3.11*"
try:
for dep_index, dep_value in enumerate(yaml_script["dependencies"]):
if re.match("python([ ><=*]+[0-9.*]*)?$", dep_value):
yaml_script["dependencies"].pop(dep_index)
break
except (KeyError, TypeError):
yaml_script["dependencies"] = []
finally:
yaml_script["dependencies"].insert(0, python_replacement_string)
print("Resulting dependencies:", yaml_script["dependencies"])
# If this prints only ['python 3.11*'], non-python dependencies were dropped.
PYRepository: Awallace3/QCMLForge
Length of output: 103
🏁 Script executed:
cat -n devtools/scripts/create_conda_env.py | sed -n '80,110p'Repository: Awallace3/QCMLForge
Length of output: 1441
🏁 Script executed:
git ls-files devtools/scripts/ | head -20Repository: Awallace3/QCMLForge
Length of output: 99
Dependency rewrite erases non-string dependencies (e.g., pip: blocks).
The code calls re.match() on dep_value without checking its type. When dependencies contain non-string entries (such as dict objects for pip: blocks), the TypeError is caught by the broad except (KeyError, TypeError) clause, which resets the entire dependencies list to [], discarding all valid packages.
Example: Input ["numpy", {"pip": ["foo==1.0"]}] becomes ['python 3.11*'].
Add type checks before regex matching:
Suggested fix
-try:
- for dep_index, dep_value in enumerate(yaml_script["dependencies"]):
- if re.match(
- "python([ ><=*]+[0-9.*]*)?$", dep_value
- ): # Match explicitly 'python' and its formats
- yaml_script["dependencies"].pop(dep_index)
- break # Making the assumption there is only one Python entry, also avoids need to enumerate in reverse
-except (KeyError, TypeError):
- # Case of no dependencies key, or dependencies: None
- yaml_script["dependencies"] = []
-finally:
- # Ensure the python version is added in. Even if the code does not need it, we assume the env does
- yaml_script["dependencies"].insert(0, python_replacement_string)
+dependencies = yaml_script.get("dependencies")
+if not isinstance(dependencies, list):
+ dependencies = []
+
+python_spec_re = re.compile(r"python([ ><=*]+[0-9.*]*)?$")
+filtered_dependencies = []
+for dep_value in dependencies:
+ if isinstance(dep_value, str) and python_spec_re.fullmatch(dep_value):
+ continue
+ filtered_dependencies.append(dep_value)
+
+# Ensure the python version is always first
+yaml_script["dependencies"] = [python_replacement_string, *filtered_dependencies]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devtools/scripts/create_conda_env.py` around lines 88 - 100, The code resets
yaml_script["dependencies"] to [] because re.match is called on non-string
dep_value and the resulting TypeError is caught by the broad except; change the
logic to first ensure yaml_script.get("dependencies") is a list and check each
dep_value is a str before calling re.match (e.g., skip non-str entries), and
narrow the exception handling to only catch KeyError (or validate types
up-front) so dict entries like pip: blocks are preserved; keep inserting
python_replacement_string at position 0 after safely removing any matching
string entry from yaml_script["dependencies"] (use yaml_script["dependencies"]
as a list and break after pop as currently done).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/CI.yaml (1)
25-28: Consider addingfail-fast: falseto the matrix strategy.With the default
fail-fast: true, if one matrix job fails, all other running jobs are cancelled. Addingfail-fast: falseallows all jobs to complete, making it easier to see all platform/version-specific failures at once.♻️ Suggested improvement
strategy: + fail-fast: false matrix: os: [macOS-latest, ubuntu-latest, windows-latest] python-version: ["3.12", "3.13"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/CI.yaml around lines 25 - 28, The CI matrix currently uses the strategy.matrix block (os/python-version) with default fail-fast behavior; add fail-fast: false under the same strategy key to prevent cancellation of other matrix jobs when one fails. Locate the strategy: block containing matrix: (the os and python-version entries) and insert fail-fast: false as a sibling to matrix so each job runs to completion and you can see all platform/version failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/CI.yaml:
- Around line 65-70: The workflow step using codecov/codecov-action@v5 needs the
upload token supplied; update the CodeCov step (the job step named "CodeCov"
that uses codecov/codecov-action@v5) to include the token input set from the
repository secret (e.g., add with: token: ${{ secrets.CODECOV_TOKEN }}),
ensuring the action receives the CODECOV token for protected-branch uploads
while keeping existing files, flags, and name inputs unchanged.
---
Nitpick comments:
In @.github/workflows/CI.yaml:
- Around line 25-28: The CI matrix currently uses the strategy.matrix block
(os/python-version) with default fail-fast behavior; add fail-fast: false under
the same strategy key to prevent cancellation of other matrix jobs when one
fails. Locate the strategy: block containing matrix: (the os and python-version
entries) and insert fail-fast: false as a sibling to matrix so each job runs to
completion and you can see all platform/version failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a97a4105-1f37-46bb-91c3-a5e88e4ddf8a
📒 Files selected for processing (3)
.github/workflows/CI.yamldocs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rstdocs/autosummary/qcmlforge.qca.setup_qcarchive_qcfractal.rst
✅ Files skipped from review due to trivial changes (2)
- docs/autosummary/qcmlforge.qca.setup_qcarchive_qcfractal.rst
- docs/autosummary/apnet_pt.AtomModels.ap2_hirshfeld_atom_model.AtomHirshfeldModel.rst
| - name: CodeCov | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| files: ./coverage.xml | ||
| flags: unittests | ||
| name: codecov-${{ matrix.os }}-py${{ matrix.python-version }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
codecov-action v5 token requirement 2024
💡 Result:
In 2024, with the release of codecov-action v5 (starting around November 2024), a token is generally required for uploads to protected branches (e.g., main/master) even in public repositories, unless the organization admin explicitly disables token authentication in Codecov settings under "Global Upload Token". This change aligns with v4 behavior but v5 introduces an opt-out for public repos. Key points: - Private repos: Token always required. - Public repos: Token required for protected branches by default; tokenless possible for fork PRs (branch prefixed like "fork:main") or if org setting allows tokenless uploads. - Common error post-upgrade: "Token required because branch is protected". - Usage: Provide via token: ${{ secrets.CODECOV_TOKEN }} in workflow, or use OIDC (use_oidc: true). - For PRs from forks/Dependabot: Tokenless supported if qualifying as unprotected branch; otherwise add token to secrets. Alternatives: OIDC authentication or disable tokens in Codecov org settings for public repos (new orgs default to not required).
Citations:
- 1: v5 upgrade gives new error
token required because branch is protectedcodecov/codecov-action#1653 - 2: Does v4 require token to be set? codecov/codecov-action#1263
- 3: https://github.com/codecov/codecov-action
- 4: [BUG] Unable to find token on PR, main branch works fine codecov/codecov-action#1764
- 5: https://docs.codecov.com/docs/codecov-tokens
- 6: https://github.com/codecov/codecov-action/blob/master/CHANGELOG.md
- 7: https://github.com/marketplace/actions/codecov
- 8: https://docs.codecov.io/docs/codecov-tokens
🏁 Script executed:
# Search for any existing CODECOV_TOKEN references in the repository
rg "CODECOV_TOKEN" --no-headingRepository: Awallace3/QCMLForge
Length of output: 45
🏁 Script executed:
# Check the CI.yaml file for complete context around codecov and any token-related configurations
cat -n .github/workflows/CI.yaml | head -80Repository: Awallace3/QCMLForge
Length of output: 2580
🏁 Script executed:
# Look for any codecov configuration files
fd -e "codecov" -e "codecov.yml" -e ".codecov.yml"Repository: Awallace3/QCMLForge
Length of output: 45
🏁 Script executed:
# Check if there are any GitHub secrets or workflow environment configurations
git ls-files | grep -E "(secret|env|config)" | head -20Repository: Awallace3/QCMLForge
Length of output: 149
Add Codecov token configuration to ensure reliable uploads.
The codecov/codecov-action@v5 requires a token for uploads to protected branches (like main) in public repositories. Without it, uploads will fail with "Token required because branch is protected."
🔧 Suggested fix
- name: CodeCov
uses: codecov/codecov-action@v5
with:
files: ./coverage.xml
flags: unittests
name: codecov-${{ matrix.os }}-py${{ matrix.python-version }}
+ token: ${{ secrets.CODECOV_TOKEN }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: CodeCov | |
| uses: codecov/codecov-action@v5 | |
| with: | |
| files: ./coverage.xml | |
| flags: unittests | |
| name: codecov-${{ matrix.os }}-py${{ matrix.python-version }} | |
| - name: CodeCov | |
| uses: codecov/codecov-action@v5 | |
| with: | |
| files: ./coverage.xml | |
| flags: unittests | |
| name: codecov-${{ matrix.os }}-py${{ matrix.python-version }} | |
| token: ${{ secrets.CODECOV_TOKEN }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/CI.yaml around lines 65 - 70, The workflow step using
codecov/codecov-action@v5 needs the upload token supplied; update the CodeCov
step (the job step named "CodeCov" that uses codecov/codecov-action@v5) to
include the token input set from the repository secret (e.g., add with: token:
${{ secrets.CODECOV_TOKEN }}), ensuring the action receives the CODECOV token
for protected-branch uploads while keeping existing files, flags, and name
inputs unchanged.
2 participants
This PR uses sphinx to build docs
Summary by CodeRabbit
Documentation
Chores
Community
Tooling
New Features