Repository: openforcefield/openff-toolkit

Issue:
`Topology.from_pdb` does not accept `pathlib.Path`
**Describe the bug**
<!-- A clear and concise description of what the bug is. -->

The toolkit sometimes handles `pathlib.Path` inputs, but `Topology.from_pdb` is [not one of those cases](https://github.com/openforcefield/openff-toolkit/blob/0.13.0/openff/toolkit/topology/topology.py#L1660), resulting in a non-obvious error.

**To Reproduce**
```python
In [1]: from openff.toolkit import Topology

In [2]: Topology.from_pdb("ALA_GLY.pdb")
Out[2]: <openff.toolkit.topology.topology.Topology at 0x16bae7b80>

In [3]: from pathlib import Path

In [4]: Topology.from_pdb(Path("ALA_GLY.pdb"))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[4], line 1
----> 1 Topology.from_pdb(Path("ALA_GLY.pdb"))

File ~/mambaforge/envs/openff-toolkit-test/lib/python3.10/site-packages/openff/utilities/utilities.py:80, in requires_package.<locals>.inner_decorator.<locals>.wrapper(*args, **kwargs)
     77 except Exception as e:
     78     raise e
---> 80 return function(*args, **kwargs)

File ~/software/openff-toolkit/openff/toolkit/topology/topology.py:1659, in Topology.from_pdb(cls, file_path, unique_molecules, toolkit_registry, _additional_substructures)
   1656 import openmm.unit as openmm_unit
   1657 from openmm.app import PDBFile
-> 1659 pdb = PDBFile(file_path)
   1661 substructure_file_path = get_data_file_path(
   1662     "proteins/aa_residues_substructures_explicit_bond_orders_with_caps.json"
   1663 )
   1665 with open(substructure_file_path, "r") as subfile:

File ~/mambaforge/envs/openff-toolkit-test/lib/python3.10/site-packages/openmm/app/pdbfile.py:95, in PDBFile.__init__(self, file, extraParticleIdentifier)
     93     inputfile = open(file)
     94     own_handle = True
---> 95 pdb = PdbStructure(inputfile, load_all_models=True, extraParticleIdentifier=extraParticleIdentifier)
     96 if own_handle:
     97     inputfile.close()

File ~/mambaforge/envs/openff-toolkit-test/lib/python3.10/site-packages/openmm/app/internal/pdbstructure.py:153, in PdbStructure.__init__(self, input_stream, load_all_models, extraParticleIdentifier)
    151 self.modified_residues = []
    152 # read file
--> 153 self._load(input_stream)

File ~/mambaforge/envs/openff-toolkit-test/lib/python3.10/site-packages/openmm/app/internal/pdbstructure.py:159, in PdbStructure._load(self, input_stream)
    157 self._reset_residue_numbers()
    158 # Read one line at a time
--> 159 for pdb_line in input_stream:
    160     if not isinstance(pdb_line, str):
    161         pdb_line = pdb_line.decode('utf-8')

TypeError: 'PosixPath' object is not iterable
```

**Output**
<!-- The full error message (may be large, that's fine. Better to paste too much than too little.) -->

**Computing environment (please complete the following information):**
 - Operating system
 - Output of running `conda list`

**Additional context**
<!-- Add any other context about the problem here. -->
I wouldn't argue too strongly this is a bug, but ensuring cases like these are handled throughout the toolkit would benefit user experience long-term.

Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/openforcefield/openff-toolkit/pull/1622?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openforcefield) Report
> Merging [#1622](https://app.codecov.io/gh/openforcefield/openff-toolkit/pull/1622?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openforcefield) (5d9231d) into [main](https://app.codecov.io/gh/openforcefield/openff-toolkit/commit/d0e11592e2485bf2fbe35b72101404a0632a6b9e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openforcefield) (d0e1159) will **decrease** coverage by `12.10%`.
> The diff coverage is `100.00%`.

<details><summary>Additional details and impacted files</summary>


</details>
mattwthompson: There are probably some other cases yet to be covered. I couldn't find other obvious file loading methods skimming a couple of files.

As is, this fixes two failures I've run into in the past week.

Patch:
diff --git a/docs/releasehistory.md b/docs/releasehistory.md
index 7d24bb4e..e1658e88 100644
--- a/docs/releasehistory.md
+++ b/docs/releasehistory.md
@@ -18,6 +18,7 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w
 
 ### Bugfixes
 - [PR #1617](https://github.com/openforcefield/openff-toolkit/pull/1617): Fixes [#1616](https://github.com/openforcefield/openff-toolkit/issues/1616), by converting NAGL charges to float and converting partial_charges to float before converting to_openeye()
+- [PR #1622](https://github.com/openforcefield/openff-toolkit/pull/1622): Fixes [#1621](https://github.com/openforcefield/openff-toolkit/issues/1621) and [#1346](https://github.com/openforcefield/openff-toolkit/issues/1347) in which some file-loading methods would fail on `pathlib.Path`.
 
 ### Improved documentation and warnings
 
diff --git a/openff/toolkit/topology/molecule.py b/openff/toolkit/topology/molecule.py
index ce42f7e9..ea41d3e5 100644
--- a/openff/toolkit/topology/molecule.py
+++ b/openff/toolkit/topology/molecule.py
@@ -1023,7 +1023,11 @@ class FrozenMolecule(Serializable):
                     loaded = True
             # TODO: Make this compatible with file-like objects (I couldn't figure out how to make an oemolistream
             # from a fileIO object)
-            if isinstance(other, str) or hasattr(other, "read") and not loaded:
+            if (
+                isinstance(other, (str, pathlib.Path))
+                or hasattr(other, "read")
+                and not loaded
+            ):
                 try:
                     mol = Molecule.from_file(
                         other,
@@ -3767,7 +3771,7 @@ class FrozenMolecule(Serializable):
     @classmethod
     def from_file(
         cls,
-        file_path,
+        file_path: Union[str, pathlib.Path, TextIO],
         file_format=None,
         toolkit_registry=GLOBAL_TOOLKIT_REGISTRY,
         allow_undefined_stereo=False,
@@ -3782,7 +3786,7 @@ class FrozenMolecule(Serializable):
 
         Parameters
         ----------
-        file_path : str or file-like object
+        file_path : str, pathlib.Path, or file-like object
             The path to the file or file-like object to stream one or more molecules from.
         file_format : str, optional, default=None
             Format specifier, usually file suffix (eg. 'MOL2', 'SMI')
@@ -3892,7 +3896,9 @@ class FrozenMolecule(Serializable):
 
         mols = list()
 
-        if isinstance(file_path, str):
+        if isinstance(file_path, (str, pathlib.Path)):
+            if isinstance(file_path, pathlib.Path):
+                file_path = file_path.as_posix()
             mols = toolkit.from_file(
                 file_path,
                 file_format=file_format,
@@ -3919,7 +3925,7 @@ class FrozenMolecule(Serializable):
     @requires_package("openmm")
     def from_polymer_pdb(
         cls,
-        file_path: Union[str, TextIO],
+        file_path: Union[str, pathlib.Path, TextIO],
         toolkit_registry=GLOBAL_TOOLKIT_REGISTRY,
         name: str = "",
     ):
@@ -3973,12 +3979,21 @@ class FrozenMolecule(Serializable):
             multiple chains or molecules.
 
         """
+        import io
+
         import openmm.unit as openmm_unit
         from openmm.app import PDBFile
 
         if isinstance(toolkit_registry, ToolkitWrapper):
             toolkit_registry = ToolkitRegistry([toolkit_registry])
 
+        if isinstance(file_path, (str, io.TextIOWrapper)):
+            pass
+        elif isinstance(file_path, pathlib.Path):
+            file_path = file_path.as_posix()
+        else:
+            raise ValueError(f"Unexpected type {type(file_path)}")
+
         pdb = PDBFile(file_path)
 
         substructure_file_path = get_data_file_path(
diff --git a/openff/toolkit/topology/topology.py b/openff/toolkit/topology/topology.py
index 05285662..7154612e 100644
--- a/openff/toolkit/topology/topology.py
+++ b/openff/toolkit/topology/topology.py
@@ -1532,7 +1532,7 @@ class Topology(Serializable):
     @requires_package("openmm")
     def from_pdb(
         cls,
-        file_path: Union[str, TextIO],
+        file_path: Union[str, Path, TextIO],
         unique_molecules: Optional[Iterable[Molecule]] = None,
         toolkit_registry=GLOBAL_TOOLKIT_REGISTRY,
         _additional_substructures: Optional[Iterable[Molecule]] = None,
@@ -1608,7 +1608,7 @@ class Topology(Serializable):
 
         Parameters
         ----------
-        file_path : str or file object
+        file_path : str, Path, or file object
             PDB information to be passed to OpenMM PDBFile object for loading
         unique_molecules : Iterable of Molecule. Default = None
             OpenFF Molecule objects corresponding to the molecules in the input
@@ -1651,11 +1651,19 @@ class Topology(Serializable):
          HierarchyElement ('B', '3', ' ', 'NME') of iterator 'residues' containing 6 atom(s)]
 
         """
+        import io
         import json
 
         import openmm.unit as openmm_unit
         from openmm.app import PDBFile
 
+        if isinstance(file_path, (str, io.TextIOWrapper)):
+            pass
+        elif isinstance(file_path, Path):
+            file_path = file_path.as_posix()
+        else:
+            raise ValueError(f"Unexpected type {type(file_path)}")
+
         pdb = PDBFile(file_path)
 
         substructure_file_path = get_data_file_path(


Test patch:
diff --git a/openff/toolkit/tests/test_molecule.py b/openff/toolkit/tests/test_molecule.py
index c8d7a788..6b9265b2 100644
--- a/openff/toolkit/tests/test_molecule.py
+++ b/openff/toolkit/tests/test_molecule.py
@@ -1022,8 +1022,11 @@ class TestMolecule:
             filename = str(outfile.name)
             ethanol.to_file(filename, file_format="sdf")
 
+            # there is different logic when file_format is None, so test both cases
             Molecule.from_file(pathlib.Path(filename))
 
+            Molecule.from_file(pathlib.Path(filename), file_format="sdf")
+
     @pytest.mark.parametrize("molecule", mini_drug_bank())
     def test_create_from_serialized(self, molecule):
         """Test standard constructor taking the output of Molecule.to_dict()."""
@@ -3958,6 +3961,23 @@ class TestMoleculeFromPDB:
     Test creation of cheminformatics-rich openff Molecule from PDB files.
     """
 
+    def test_from_pdb_input_types(self):
+        import pathlib
+
+        import openmm.app
+
+        protein_path = get_data_file_path("proteins/ace-ala-nh2.pdb")
+
+        Molecule.from_polymer_pdb(protein_path)
+
+        Molecule.from_polymer_pdb(pathlib.Path(protein_path))
+
+        with open(protein_path) as f:
+            Molecule.from_polymer_pdb(f)
+
+        with pytest.raises(ValueError, match="Unexpected type.*PDBFile"):
+            Molecule.from_polymer_pdb(openmm.app.PDBFile(protein_path))
+
     # TODO: Implement all the tests
     def test_from_pdb_t4_n_atoms(self):
         """Test off Molecule contains expected number of atoms from T4 pdb."""
diff --git a/openff/toolkit/tests/test_topology.py b/openff/toolkit/tests/test_topology.py
index 48399c2b..c8cfac7c 100644
--- a/openff/toolkit/tests/test_topology.py
+++ b/openff/toolkit/tests/test_topology.py
@@ -780,6 +780,24 @@ class TestTopology:
         assert top.molecule(17).is_isomorphic_with(Molecule.from_smiles("[Br-]"))
         assert top.molecule(18).is_isomorphic_with(Molecule.from_smiles("[I-]"))
 
+    @requires_rdkit
+    def test_from_pdb_input_types(self):
+        import pathlib
+
+        import openmm.app
+
+        protein_path = get_data_file_path("proteins/ace-ala-nh2.pdb")
+
+        Topology.from_pdb(protein_path)
+
+        Topology.from_pdb(pathlib.Path(protein_path))
+
+        with open(protein_path) as f:
+            Topology.from_pdb(f)
+
+        with pytest.raises(ValueError, match="Unexpected type.*PDBFile"):
+            Topology.from_pdb(openmm.app.PDBFile(protein_path))
+
     @requires_rdkit
     def test_from_pdb_two_polymers_metadata(self):
         """Test that a PDB with two capped polymers is loaded correctly"""