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.

Sign up for GitHub

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

Jump to bottom

Fixing the conda cron #398

Merged
merged 7 commits into from
Nov 15, 2024
Merged

Fixing the conda cron #398

merged 7 commits into from
Nov 15, 2024

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Nov 12, 2024 •
edited
Loading

This should fix any issues we've had lately.

@codecov Codecov
Copy link

codecov bot commented Nov 12, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.65%. Comparing base (5cdb748) to head (2cdd5f9).
Report is 8 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #398   +/-   ##
=======================================
  Coverage   98.65%   98.65%           
=======================================
  Files          36       36           
  Lines        2075     2075           
=======================================
  Hits         2047     2047           
  Misses         28       28

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

@IAlibay
Copy link
Member Author

IAlibay commented Nov 12, 2024

Now it's failing properly.

@mikemhenry
Copy link
Contributor 

2024-11-12T13:16:04.6116660Z >       assert in_lines == out_lines
2024-11-12T13:16:04.6117140Z E       AssertionError: assert ['HETATM    1...  H  \n', ...] == ['HETATM    1...  H  \n', ...]
2024-11-12T13:16:04.6117550Z E         
2024-11-12T13:16:04.6118210Z E         At index 14 diff: 'ATOM     15  OE2 GLU A   2      68.596  19.156  21.677  1.00  0.00           O1-\n' != 'ATOM     15  OE2 GLU A   2      68.596  19.156  21.677  1.00  0.00           O  \n'
2024-11-12T13:16:04.6118850Z E         
2024-11-12T13:16:04.6119020Z E         Full diff:
2024-11-12T13:16:04.6119210Z E           [
2024-11-12T13:16:04.6119530Z E               'HETATM    1  CH3 ACE A   1      69.744  11.567  19.362  1.00  '
2024-11-12T13:16:04.6119910Z E               '0.00           C  \n',
2024-11-12T13:16:04.6120290Z E               'HETATM    2  C   ACE A   1      69.053  12.763  20.004  1.00  '
2024-11-12T13:16:04.6120650Z E               '0.00           C  \n',
2024-11-12T13:16:04.6121030Z E               'HETATM    3  O   ACE A   1      67.851  12.703  20.259  1.00  '
2024-11-12T13:16:04.6121390Z E               '0.00           O  \n',
2024-11-12T13:16:04.6121770Z E               'HETATM    4  H1  ACE A   1      69.181  11.231  18.492  1.00  '
2024-11-12T13:16:04.6122140Z E               '0.00           H  \n',
2024-11-12T13:16:04.6122510Z E               'HETATM    5  H2  ACE A   1      70.755  11.816  19.037  1.00  '
2024-11-12T13:16:04.6123110Z E               '0.00           H  \n',
2024-11-12T13:16:04.6123490Z E               'HETATM    6  H3  ACE A   1      69.806  10.743  20.072  1.00  '
2024-11-12T13:16:04.6123850Z E               '0.00           H  \n',
2024-11-12T13:16:04.6124220Z E               'ATOM      7  N   GLU A   2      69.807  13.849  20.242  1.00  '
2024-11-12T13:16:04.6124580Z E               '0.00           N  \n',
2024-11-12T13:16:04.6125080Z E               'ATOM      8  CA  GLU A   2      69.255  15.135  20.671  1.00  '
2024-11-12T13:16:04.6125460Z E               '0.00           C  \n',
2024-11-12T13:16:04.6125820Z E               'ATOM      9  C   GLU A   2      68.759  15.888  19.424  1.00  '
2024-11-12T13:16:04.6126180Z E               '0.00           C  \n',
2024-11-12T13:16:04.6126560Z E               'ATOM     10  O   GLU A   2      69.474  15.958  18.424  1.00  '
2024-11-12T13:16:04.6126920Z E               '0.00           O  \n',
2024-11-12T13:16:04.6127280Z E               'ATOM     11  CB  GLU A   2      70.305  15.908  21.501  1.00  '
2024-11-12T13:16:04.6127650Z E               '0.00           C  \n',
2024-11-12T13:16:04.6128020Z E               'ATOM     12  CG  GLU A   2      69.757  17.154  22.248  1.00  '
2024-11-12T13:16:04.6128390Z E               '0.00           C  \n',
2024-11-12T13:16:04.6128760Z E               'ATOM     13  CD  GLU A   2      69.581  18.433  21.417  1.00  '
2024-11-12T13:16:04.6129130Z E               '0.00           C  \n',
2024-11-12T13:16:04.6129490Z E               'ATOM     14  OE1 GLU A   2      70.425  18.679  20.530  1.00  '
2024-11-12T13:16:04.6129860Z E               '0.00           O  \n',
2024-11-12T13:16:04.6130220Z E               'ATOM     15  OE2 GLU A   2      68.596  19.156  21.677  1.00  '
2024-11-12T13:16:04.6130590Z E         -     '0.00           O  \n',
2024-11-12T13:16:04.6130840Z E         ?                      ^^
2024-11-12T13:16:04.6131110Z E         +     '0.00           O1-\n',
2024-11-12T13:16:04.6131360Z E         ?                      ^^
2024-11-12T13:16:04.6131720Z E               'ATOM     16  H   GLU A   2      70.789  13.841  20.010  1.00  '

Probably some dep changed something upstream, since things are going from N1 to N for example.

@atravitz atravitz self-assigned this Nov 12, 2024
@atravitz
Copy link
Contributor

I was not able to reproduce this error locally on my M1 macbook (macos 14.6.1)

@mikemhenry
Copy link
Contributor

mikemhenry commented Nov 13, 2024 •
edited
Loading

I was able to reproduce the issue locally.

(and also this one openforcefield/openff-utilities#89 )

@atravitz can you see if you get the same error using this env?
https://gist.github.com/mikemhenry/da68774628a7196151c3680893c0a3df

After you create and activate the env, see if pytest --pyargs gufe gives you the same error

% micromamba info

                                           __
          __  ______ ___  ____ _____ ___  / /_  ____ _
         / / / / __ `__ \/ __ `/ __ `__ \/ __ \/ __ `/
        / /_/ / / / / / / /_/ / / / / / / /_/ / /_/ /
       / .___/_/ /_/ /_/\__,_/_/ /_/ /_/_.___/\__,_/
      /_/


            environment : gufe-ci (active)
           env location : /Users/lilmac/micromamba/envs/gufe-ci
      user config files : /Users/lilmac/.mambarc
 populated config files : /Users/lilmac/.condarc
       libmamba version : 0.25.0
     micromamba version : 0.25.1
           curl version : libcurl/7.76.1 SecureTransport (OpenSSL/1.1.1q) zlib/1.2.12 libssh2/1.9.0 nghttp2/1.47.0
     libarchive version : libarchive 3.3.3 zlib/1.2.11 liblzma/5.2.5 bz2lib/1.0.8 liblz4/1.9.2 libzstd/1.4.5
       virtual packages : __unix=0=0
                          __osx=13.5.2=0
                          __archspec=1=arm64
               channels : https://conda.anaconda.org/conda-forge/osx-arm64
                          https://conda.anaconda.org/conda-forge/noarch
       base environment : /Users/lilmac/micromamba
               platform : osx-arm64

@mikemhenry
Copy link
Contributor

my hunch is still some upstream package changed since we run these pdb round trips on PRs

@atravitz
Copy link
Contributor

my hunch is still some upstream package changed since we run these pdb round trips on PRs

Yep, it's openmm 8.2.0. I was able to reproduce the error using mike's env, and then resolve the error by just downgrading openmm to 8.1.2.

@mikemhenry
Copy link
Contributor

Ah openmm isn't a direct gufe dependency, but openff-toolkit is, which pulls in openff-interchange

@mikemhenry
Copy link
Contributor

Actually it is a direct dependency, but we don't list it in our recipe, and have just gotten lucky it gets pulled in, see:
https://github.com/conda-forge/gufe-feedstock/blob/main/recipe/meta.yaml
and
https://github.com/OpenFreeEnergy/gufe/blob/main/gufe/components/proteincomponent.py#L12

@mikemhenry
Copy link
Contributor

I bet it is this PR
openmm/openmm#4630

@mikemhenry
Copy link
Contributor

Looking at that PR, I think we just need to update our tests since the new behavior is correct.

This PR: #406 confirms that it is with 8.2.

I think we just need to update the test "reference" PDB to be the new "correct" one

@IAlibay
Copy link
Member Author

IAlibay commented Nov 14, 2024

Thanks for looking into this @mikemhenry - I agree, the formal charges make a lot of sense / adhere better to reality.

My propose plan is:

  1. For now we pin gufe to openmm < 8.2 on conda-forge
  2. We merge this PR
  3. We pin openmm < 8.2 in the environment.yaml file here too so that we don't halt other PRs
  4. We follow up with another issue/PR to fix things for openmm 8.2

Do you both agree @atravitz and @mikemhenry ?

@atravitz
Copy link
Contributor

related: conda-forge/gufe-feedstock#40

@atravitz atravitz merged commit d15ea96 into main Nov 15, 2024
16 checks passed
@atravitz atravitz deleted the fix-cron branch November 15, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atravitz atravitz atravitz approved these changes

@mikemhenry mikemhenry Awaiting requested review from mikemhenry

Assignees

@atravitz atravitz

Labels
None yet
Projects
gufe : advancement sprints
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IAlibay @mikemhenry @atravitz