Skip to content

Use openff-packmol#1423

Open
mattwthompson wants to merge 18 commits intomainfrom
new-packmol-wrapper
Open

Use openff-packmol#1423
mattwthompson wants to merge 18 commits intomainfrom
new-packmol-wrapper

Conversation

@mattwthompson
Copy link
Copy Markdown
Member

@mattwthompson mattwthompson commented Feb 5, 2026

Description

Checklist

  • Add tests
  • Lint
  • Update docstrings

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 5, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (cbc0179) to head (d68088a).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
openff/interchange/components/_packmol.py 0.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (cbc0179) and HEAD (d68088a). Click for more details.

HEAD has 16 uploads less than BASE
Flag BASE (cbc0179) HEAD (d68088a)
22 6
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #1423       +/-   ##
==========================================
- Coverage   93.97%   0.00%   -93.98%     
==========================================
  Files          73      73               
  Lines        6340    6046      -294     
==========================================
- Hits         5958       0     -5958     
- Misses        382    6046     +5664     

☔ View full report in Codecov by Sentry.
📢 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.

pixi.toml Outdated
[feature.etc.dependencies]
# a Packmol install separate from AmberTools is needed to test PBCs
packmol = ">=20.15.0"
openff-packmol = "*"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a slight-of-hand of complexity; AmberTools forbids Packmol from being installed alongside it, so this can only go into the Pixi feature I designed around not having AmberTools.

This ultimately needs to be dealt with at the packaging level

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@mattwthompson mattwthompson changed the title Stage refactor to openff-packmol Use openff-packmol Mar 27, 2026
@mattwthompson
Copy link
Copy Markdown
Member Author

Blocked by conda-forge/openff-packmol-feedstock#1

@mattwthompson mattwthompson marked this pull request as ready for review April 3, 2026 16:14
@mattwthompson mattwthompson requested a review from Copilot April 3, 2026 16:14
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates PACKMOL-related functionality in openff-interchange to use the external openff-packmol package, updating imports, environments, and CI configuration accordingly.

Changes:

  • Replaced the in-repo PACKMOL wrapper implementation with a re-export shim pointing to openff.packmol.
  • Updated Pixi environments/features and dependencies to install openff-packmol and (optionally) the packmol executable.
  • Updated tests and example notebooks to import PACKMOL utilities from openff.packmol instead of openff.interchange.components._packmol.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pyproject.toml Adds openff.packmol.* to mypy ignored imports.
pixi.toml Pins openff-interchange-base to 0.5.*, adds openff-packmol, introduces a packmol feature group, and updates env definitions.
openff/interchange/components/_packmol.py Replaces the local implementation with imports/re-exports from openff.packmol._packmol.
openff/interchange/_tests/unit_tests/interop/openmm/_import/test_virtual_sites.py Switches solvate_topology import to openff.packmol.
openff/interchange/_tests/unit_tests/components/test_packmol.py Removes the in-repo packmol unit test suite.
openff/interchange/_tests/test_issues.py Switches PACKMOL-related imports to openff.packmol.
openff/interchange/_tests/interoperability_tests/components/test_packmol.py Switches to openff.packmol (and _find_packmol from its private module).
examples/protein_ligand/protein_ligand.ipynb Updates imports to use openff.packmol.
examples/packed_box/packed_box.ipynb Updates imports to use openff.packmol.
examples/ligand_in_water/ligand_in_water.ipynb Updates imports to use openff.packmol.
.github/workflows/ci.yaml Removes etc environments from the matrix and enables Pixi frozen installs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -2,1161 +2,60 @@
A wrapper around PACKMOL. Adapted from OpenFF Evaluator v0.4.3.
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module docstring is now misleading: this file no longer implements a Packmol wrapper (it re-exports symbols from openff-packmol). Please update the docstring to reflect that it’s a compatibility re-export (and, if intended, note any deprecation/migration guidance).

Suggested change
A wrapper around PACKMOL. Adapted from OpenFF Evaluator v0.4.3.
Compatibility re-export of PACKMOL utilities from ``openff.packmol._packmol``.
This module no longer implements the PACKMOL wrapper directly; it re-exports
symbols to preserve the historical import path. New code should prefer importing
from ``openff.packmol._packmol`` directly.

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +9
from openff.packmol._packmol import (
RHOMBIC_DODECAHEDRON,
RHOMBIC_DODECAHEDRON_XYHEX,
UNIT_CUBE,
_box_from_density,
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module imports everything from openff.packmol._packmol, which is a private (underscore-prefixed) module. Relying on private APIs increases the chance of breakage on openff-packmol updates; prefer importing from the public openff.packmol namespace (or add a small shim here that only depends on public exports).

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 4
import pytest
from openff.packmol import solvate_topology_nonwater
from openff.packmol._packmol import _find_packmol
from openff.toolkit import Quantity
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test now imports _find_packmol from openff.packmol._packmol, which is a private module. To reduce coupling to openff-packmol internals, consider using a public executable check (e.g., openff.utilities.has_executable("packmol") or shutil.which) for the skip condition instead of importing a private helper.

Copilot uses AI. Check for mistakes.
pixi.toml Outdated
Comment on lines +98 to +104
nglview = "*"
jax = "*"
pyedr = "*"
openff-nagl-base = "*"
openff-nagl-base = ">=0.5.2"
openff-nagl-models = "*"
openff-packmol = "*"
# smirnoff-plugins = "2025.*" # https://github.com/conda-forge/smirnoff-plugins-feedstock/pull/23
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openff.interchange.components._packmol now imports openff.packmol, but openff-packmol is only listed under the test feature dependencies here. If this module is part of the supported runtime API, consider moving openff-packmol into the base dependencies (or the packmol feature) so non-test environments that import it don’t fail.

Copilot uses AI. Check for mistakes.
Comment on lines +34 to 61
__all__ = (
"RHOMBIC_DODECAHEDRON",
"RHOMBIC_DODECAHEDRON_XYHEX",
"UNIT_CUBE",
"_box_from_density",
"_box_vectors_are_in_reduced_form",
"_build_input_file",
"_center_topology_at",
"_check_add_positive_mass",
"_check_box_shape_shape",
"_compute_brick_from_box_vectors",
"_create_molecule_pdbs",
"_create_solute_pdb",
"_find_packmol",
"_get_packmol_version",
"_iter_lattice_vecs",
"_load_positions",
"_max_dist_between_points",
"_range_neg_pos",
"_scale_box",
"_unit_vec",
"_validate_inputs",
"_wrap_into",
"_wrap_into_brick",
"pack_box",
"solvate_topology",
"solvate_topology_nonwater",
)
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this file is now a compatibility re-export layer, it would be good to add/keep a small unit test that imports openff.interchange.components._packmol and asserts key symbols (e.g., pack_box, solvate_topology) are available. The previous packmol-specific unit tests were removed, so this backward-compatibility path is currently untested in this repo.

Copilot uses AI. Check for mistakes.
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.

2 participants