Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump to python 3.12 #1180

Merged
merged 7 commits into from
May 1, 2024
Merged

Conversation

CalCraven
Copy link
Contributor

PR Summary:

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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
@@           Coverage Diff           @@
##             main    #1180   +/-   ##
=======================================
  Coverage   87.32%   87.32%           
=======================================
  Files          62       62           
  Lines        6525     6525           
=======================================
  Hits         5698     5698           
  Misses        827      827           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chrisjonesBSU
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

It looks like the MacOS and python 3.12 issue is fixed

@CalCraven
Copy link
Contributor Author

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

@chrisjonesBSU
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

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

I think this is good to go.

@daico007 daico007 merged commit 8495605 into mosdef-hub:main May 1, 2024
19 checks passed
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.

3 participants