mosdef-hub / mbuild

A hierarchical, component based molecule builder
https://mbuild.mosdef.org
Other
171 stars 79 forks source link

Bump to python 3.12 #1180

Closed CalCraven closed 3 months ago

CalCraven commented 4 months ago

PR Summary:

PR Checklist


codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.32%. Comparing base (2bb047e) to head (be05bf3). Report is 3 commits behind head on main.

:exclamation: Current head be05bf3 differs from pull request most recent head c9f7def. Consider uploading reports for the commit c9f7def to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1180 +/- ## ======================================= Coverage 87.32% 87.32% ======================================= Files 62 62 Lines 6525 6525 ======================================= Hits 5698 5698 Misses 827 827 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

chrisjonesBSU commented 4 months ago

Looks like the latest release of mdtraj on conda doesn't support python 3.12 on MacOS environments. There has been some activity on the mdtraj feedstock related to OSX and python 3.12 about 5 months ago.

chrisjonesBSU commented 4 months ago

Adding some notes from the dev call. I think we can remove mdtraj as a dependency if we

1) Change the default backend of mol2 to gmso 2) Change the default backend of pdb to parmed 3) Remove the outdated hoomdxml file format

chrisjonesBSU commented 4 months ago

After looking through some of the unit tests, I looks like using mdtraj for loading pdb's instead of parmed does have some benefits for setting the expected hierarchy of compounds? I think its worth considering before we clean up and condense the reader's in mbuild.

CalCraven commented 4 months ago

I ran this script to look at the difference in hierachy of mbuid objects:

import mbuild as mb
from mbuild.utils.io import get_fn
import parmed

protein1 = mb.load(get_fn("6m03.pdb"))
protein2 = mb.Compound()
protein2.from_parmed(parmed.load_file(get_fn("6m03.pdb"))
print(protein1.children)
print("#"*20)
print(protein1.children[0].children)
print("#"*20)
print(protein2.children)

output

[<Compound 2367 particles, 2420 bonds, non-periodic, id: 4540138448>,
 <Compound 87 particles, 0 bonds, non-periodic, id: 6406571344>]
############################################
[<SER 6 particles, 5 bonds, non-periodic, id: 6411784656>,
 <GLY 4 particles, 3 bonds, non-periodic, id: 6412703568>,
 <PHE 11 particles, 11 bonds, non-periodic, id: 6415382672>,
 ...
]
############################################
[<SER 6 particles, 5 bonds, non-periodic, id: 6411784656>,
 <GLY 4 particles, 3 bonds, non-periodic, id: 6412703568>,
 <PHE 11 particles, 11 bonds, non-periodic, id: 6415382672>,
 ...
 <HOH 1 particles, 0 bonds, non-periodic, id: 6412399440>,
 <HOH 1 particles, 0 bonds, non-periodic, id: 6412388688>,
 <HOH 1 particles, 0 bonds, non-periodic, id: 6412444368>,
 <HOH 1 particles, 0 bonds, non-periodic, id: 6412441488>,
 ...
 <HOH 1 particles, 0 bonds, non-periodic, id: 6412594896>]

One of the huge issues here is that parmed doesn't add the hydrogens, so it looks like a single atom. The other issue is the containment of the compounds. Maybe we can adjust that using some of the condense/flatten mBuild methods as part of the writer.

CalCraven commented 4 months ago

Well it also looks like print(protein1.children[1].children) gives just the oxygen particles only. So that's just the current behavior with hydrogen implicit pdbs I guess. So less worried about that then.

CalCraven commented 4 months ago

We may be able to wait this out as mdtraj is close to full support for 3.12, which renders switching the backend to parmed from mdtraj moot.

chrisjonesBSU commented 4 months ago

It looks like the MacOS and python 3.12 issue is fixed

CalCraven commented 4 months ago

Have a flaky test with mac os 3.11 now. Something off with the pybel smiles string for a benzene ring.

chrisjonesBSU commented 4 months ago

Have a flaky test with mac os 3.11 now. Something off with the pybel smiles string for a benzene ring.

I think its the Ubuntu 3.11 test.

Also, I think the macos-latest is pulling arm images of MacOS. See this recent PR in mdtraj. I think we should use macos-13 to test on x86.

Should we include both macos-latest and macos-13 in the os-matrix?

chrisjonesBSU commented 4 months ago

Here is the output of micromamba info from one of the MacOS actions:

micromamba info
  /Users/runner/micromamba-bin/micromamba info -r /Users/runner/micromamba -n mbuild-dev

         libmamba version : 1.5.8
       micromamba version : 1.5.8
             curl version : libcurl/8.6.0 SecureTransport (OpenSSL/3.2.1) zlib/1.2.13 zstd/1.5.5 libssh2/1.11.0 nghttp2/1.58.0
       libarchive version : libarchive 3.7.2 zlib/1.2.13 bz2lib/1.0.8 libzstd/1.5.5
         envs directories : /Users/runner/micromamba/envs
            package cache : /Users/runner/micromamba/pkgs
                            /Users/runner/.mamba/pkgs
              environment : mbuild-dev
             env location : /Users/runner/micromamba/envs/mbuild-dev
        user config files : /Users/runner/.mambarc
   populated config files : /Users/runner/work/_temp/setup-micromamba/.condarc
         virtual packages : __unix=0=0
                            __osx=14.4.1=0
                            __archspec=1=arm64
                 channels : https://conda.anaconda.org/conda-forge/osx-arm64
                            https://conda.anaconda.org/conda-forge/noarch
         base environment : /Users/runner/micromamba
                 platform : osx-arm64