Add small molecule repex consistency tests #1065

zhang-ivy · 2022-06-30T22:21:35Z

Description

Adds neutral and charge changing small molecule tests ~~(the latter one has not been added yet)~~.

Motivation and context

Resolves #959
Resolves #1061

How has this been tested?

Change log

zhang-ivy · 2022-07-02T17:31:51Z

@ijpulidos : I've pushed the small molecule neutral and charge tests. The charge test isn't done yet, but I was thinking you could take over from here? The charge test is currently yielding the error described here. Once we figure out how to fix that error, you may have to debug a bit more to make sure the test is running without any more errors and then check that its actually passing.

Note that I had to make some changes to RelativeFEPSetup to make sure that the host (cb7.mol2) can be parametrized properly. These changes are necessary because otherwise RelativeFEPSetup does not consider the host as a small molecule and therefore does not tell the SystemGenerator that it needs to parametrize it as a small molecule.

…/perses into add-small-molecule-test

zhang-ivy · 2022-07-06T13:16:30Z

@ijpulidos : Update: I've fixed the parametrization error in the charge test that I mentioned above

ijpulidos · 2022-07-06T14:01:25Z

@zhang-ivy Thanks for this! I'll take a look and let you know if I encounter anything.

ijpulidos · 2022-07-18T20:50:35Z

We need to switch to the new LangevinMiddelIntegrator once we cut a new release of openmmtools.

zhang-ivy · 2022-07-18T20:54:57Z

To speed up this test, see #1058 (comment)

ijpulidos · 2022-08-15T16:52:24Z

To use the change in openmmtools 0.21.5 that uses LangevinMiddleIntegrator we would have to change the test to use LangevinDynamicsMove instead of LangevinSplittingDynamicsMove, but I see that almost all tests (except one) use the latter. How do we want to approach this? Just changing the move in this test would be enough to test what we want to test?

ijpulidos · 2022-08-15T17:00:08Z

How do we want to approach this? Just changing the move in this test would be enough to test what we want to test?

From discussions with @zhang-ivy , I think we want to change all of the tests to use LangevinDynamicsMove which in turn would use the LangevinMiddleIntegrator changes in latest openmmtools.

…Adds gpu tests decorator.

codecov · 2022-08-15T17:58:10Z

Codecov Report

Merging #1065 (7b16afc) into main (6730065) will decrease coverage by 0.32%.
The diff coverage is 3.90%.

ijpulidos · 2022-08-15T21:02:31Z

As discussed in our dev sync, I saw the following results

According to log output it would be taking more than 4 hours for completion, which seems like too much compared to what @zhang-ivy has obtained in other tests.

The script to generate the data is attached. I basically just copied the test_RESTCapableHybridTopologyFactory_repex_charge_transformation and made it a script. To run you can use python test_repex_charge.py 2> stderr_out.txt for example, this would write the stderr output to the specified file. You can generate the plots (assuming you have two log files you want to compare), using the attached make_plots.py barebones script (modify as needed).

We should also try to run this using the nightly versions of Openmm to see further performance boosts.

test_repex_charge.tar.gz

…ore.

…s into add-small-molecule-test

jchodera · 2022-08-17T02:50:25Z

perses/tests/test_repex.py

@@ -54,16 +54,15 @@ def test_RESTCapableHybridTopologyFactory_repex_neutral_mutation():
            # Set up repex simulation
            reporter_file = os.path.join(temp_dir, f"{wt_name}-{mutant_name}.nc")
            reporter = MultiStateReporter(reporter_file, checkpoint_interval=10)


We can set the checkpoint_interval to be 100 here. There's no need to checkpoint this frequently.

Actually, we don't ever use the positions, so we probably don't need to write to the checkpoint file at all here.
The energies should be written every iteration regardless of the checkpoint_interval.

using 100 here for the checkpoint_interval worked okay in the preliminary tests

jchodera · 2022-08-17T02:50:44Z

perses/tests/test_repex.py

@@ -157,16 +155,15 @@ def test_RESTCapableHybridTopologyFactory_repex_charge_mutation():
                # Set up repex simulation
                reporter_file = os.path.join(temp_dir, f"{wt_name}-{mutant_name}.nc")
                reporter = MultiStateReporter(reporter_file, checkpoint_interval=10)


We can set the checkpoint_interval to 100 here.

perses/tests/test_repex.py

mikemhenry · 2023-04-20T06:34:22Z

Tests took 6hr30min https://github.com/choderalab/perses/actions/runs/4747425819/jobs/8432377018

with only one failure:

FAILED perses/tests/test_repex.py::test_RESTCapableHybridTopologyFactory_repex_charge_mutation - FileNotFoundError: [Errno 2] No such file or directory: '/actions-runner/_work/perses/perses/perses/data/ala_solvated.cif'

ijpulidos · 2023-04-20T15:37:18Z

@mikemhenry that's great news. The failing test is just that we have the inverse order in the transformation, I made the changes but forgot to push them, these will add a few hours to the tests but I think we should be okay. I'll push them in a second.

.github/workflows/self-hosted-gpu-test.yml

mikemhenry · 2023-04-21T16:21:05Z

.github/workflows/self-hosted-gpu-test.yml

    env:
      TEST_MODE: GPU
-      OPENMM: ${{ matrix.cfg.openmm }}
+      OPENMM: 8.0


We will need this for when we merge in #1186

zhang-ivy · 2023-04-21T19:50:56Z

@ijpulidos : I just reviewed again and your changes look good to me.

A couple of other things to address:

ijpulidos · 2023-04-23T20:30:02Z

Tests took 6hr30min https://github.com/choderalab/perses/actions/runs/4747425819/jobs/8432377018

with only one failure:
FAILED perses/tests/test_repex.py::test_RESTCapableHybridTopologyFactory_repex_charge_mutation - FileNotFoundError: [Errno 2] No such file or directory: '/actions-runner/_work/perses/perses/perses/data/ala_solvated.cif'

@mikemhenry Last run of the manually run GPU CI tests was successful https://github.com/choderalab/perses/actions/runs/4766605648/jobs/8473881768#step:9:571

@zhang-ivy thanks for the comments/review. I'll be adressing these as soon as possible.

jchodera

The GPU tests took 10 hours (~$5 per run), with 8.5 hours spent in these two tests:

16633.61s call     perses/tests/test_repex.py::test_RESTCapableHybridTopologyFactory_repex_charge_mutation
13661.38s call     perses/tests/test_repex.py::test_RESTCapableHybridTopologyFactory_repex_charge_transformation

Are we sure the tests are running on the GPU? We aren't logging debug output that would report the Platform that the tests are running on, but we should probably confirm this.
Is there any way we can reduce the system size for these tests so that runtime is shortened?
I've highlighted lines where we might be able to halve the number of steps per iteration, which would cut the runtime in half.

If we can't reduce the cost drastically, we might have to only enable these tests in special circumstances, such as once per day on main.

perses/tests/test_repex.py

…s into add-small-molecule-test

ijpulidos · 2023-04-27T20:53:50Z

Good news, we can use 125 steps per iteration and still have the tests passing, we probably could do a bit of more refining here to lower the walltimes, but we moved from ~10 hours, to ~6.5 hours in the latest manual run of these tests here.

ijpulidos · 2023-04-27T20:56:33Z

And as we discussed in our dev sync, these tests would only be run on a nightly basis, and maybe we can discuss about running them less frequently if we need to. Monitoring the costs here shouldn't be hard on the AWS dashboard.

.github/workflows/self-hosted-gpu-test.yml

zhang-ivy added 5 commits June 24, 2022 11:23

add neutral test

f48a588

add small molecule tests

4f82361

remove charge test

6c035db

add charge test

74bcf38

add guest mol2s

6e8893b

zhang-ivy and others added 3 commits July 6, 2022 08:43

Merge branch 'main' into add-small-molecule-test

6a0ff93

fix charge changing test

406fe70

Merge branch 'add-small-molecule-test' of ssh://github.com/choderalab…

19a3796

…/perses into add-small-molecule-test

zhang-ivy changed the title ~~[WIP] Add small molecule repex consistency tests~~ Add small molecule repex consistency tests Jul 6, 2022

zhang-ivy added 2 commits July 6, 2022 10:03

add cache

5590a3c

make sure lrc is set correctly

f39c3af

zhang-ivy added this to the 0.10.2 Bugfix release milestone Jul 18, 2022

ijpulidos added 2 commits August 15, 2022 13:22

Small code cleanup. Fix docstring. Remove unused imports.

82f2960

Using LangevinDynamicsMove instead of LangevinSplittingDynamicsMove. …

9356b63

…Adds gpu tests decorator.

Merge branch 'main' into add-small-molecule-test

4c5ab63

ijpulidos added 2 commits August 16, 2022 14:47

Only using LangevinDynamicsMove in tests. Others stay the same as bef…

105e71b

…ore.

Merge branch 'add-small-molecule-test' of github.com:choderalab/perse…

4406b4a

…s into add-small-molecule-test

jchodera reviewed Aug 17, 2022

View reviewed changes

perses/tests/test_repex.py Show resolved Hide resolved

jchodera reviewed Aug 17, 2022

View reviewed changes

perses/tests/test_repex.py Show resolved Hide resolved

ijpulidos and others added 4 commits April 18, 2023 14:21

Swapping tests, now correct order.

d953e62

use micromamba

280ebbb

micromamba thowing a fit about oPtIonS iT dOeSn'T uNDerStAnd

af484f6

Merge branch 'main' into add-small-molecule-test

b66dd53

ijpulidos added 4 commits April 20, 2023 09:47

Changing back the transformation order to the correct one.

c9bdae9

Correct comparison charge mutation. Naming cleanup

629b20e

Better naming. Minor indenting cleanup.

1f7e414

Removing misplaced quotes, my bad!

7e5be5b

mikemhenry reviewed Apr 21, 2023

View reviewed changes

.github/workflows/self-hosted-gpu-test.yml Outdated Show resolved Hide resolved

Update .github/workflows/self-hosted-gpu-test.yml

15f24c0

mikemhenry reviewed Apr 21, 2023

View reviewed changes

jchodera reviewed Apr 23, 2023

View reviewed changes

perses/tests/test_repex.py Outdated Show resolved Hide resolved

perses/tests/test_repex.py Outdated Show resolved Hide resolved

perses/tests/test_repex.py Outdated Show resolved Hide resolved

perses/tests/test_repex.py Outdated Show resolved Hide resolved

ijpulidos added 5 commits April 26, 2023 10:43

Merge branch 'add-small-molecule-test' of github.com:choderalab/perse…

2467f82

…s into add-small-molecule-test

Removing deprecated comment

2fd2a2a

Lowering the number of steps per iteration to decrease run times.

693f41c

Merge branch 'main' into add-small-molecule-test

ad8d805

removing commented/unused line

aa6b2c6

ijpulidos requested a review from mikemhenry April 27, 2023 20:57

mikemhenry reviewed Apr 28, 2023

View reviewed changes

.github/workflows/self-hosted-gpu-test.yml Outdated Show resolved Hide resolved

Update .github/workflows/self-hosted-gpu-test.yml

7b16afc

mikemhenry enabled auto-merge (squash) April 28, 2023 17:45

mikemhenry approved these changes Apr 28, 2023

View reviewed changes

mikemhenry merged commit 8e3e423 into main Apr 28, 2023

mikemhenry deleted the add-small-molecule-test branch April 28, 2023 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add small molecule repex consistency tests #1065

Add small molecule repex consistency tests #1065

zhang-ivy commented Jun 30, 2022 •

edited

Loading

zhang-ivy commented Jul 2, 2022 •

edited

Loading

zhang-ivy commented Jul 6, 2022

ijpulidos commented Jul 6, 2022

ijpulidos commented Jul 18, 2022

zhang-ivy commented Jul 18, 2022

ijpulidos commented Aug 15, 2022 •

edited

Loading

ijpulidos commented Aug 15, 2022

codecov bot commented Aug 15, 2022 •

edited

Loading

ijpulidos commented Aug 15, 2022

jchodera Aug 17, 2022

zhang-ivy Aug 17, 2022

ijpulidos Aug 19, 2022

jchodera Aug 17, 2022

mikemhenry commented Apr 20, 2023

ijpulidos commented Apr 20, 2023

mikemhenry Apr 21, 2023

zhang-ivy commented Apr 21, 2023

ijpulidos commented Apr 23, 2023

jchodera left a comment

ijpulidos commented Apr 27, 2023

ijpulidos commented Apr 27, 2023

Add small molecule repex consistency tests #1065

Add small molecule repex consistency tests #1065

Conversation

zhang-ivy commented Jun 30, 2022 • edited Loading

Description

Motivation and context

How has this been tested?

Change log

zhang-ivy commented Jul 2, 2022 • edited Loading

zhang-ivy commented Jul 6, 2022

ijpulidos commented Jul 6, 2022

ijpulidos commented Jul 18, 2022

zhang-ivy commented Jul 18, 2022

ijpulidos commented Aug 15, 2022 • edited Loading

ijpulidos commented Aug 15, 2022

codecov bot commented Aug 15, 2022 • edited Loading

Codecov Report

ijpulidos commented Aug 15, 2022

jchodera Aug 17, 2022

Choose a reason for hiding this comment

zhang-ivy Aug 17, 2022

Choose a reason for hiding this comment

ijpulidos Aug 19, 2022

Choose a reason for hiding this comment

jchodera Aug 17, 2022

Choose a reason for hiding this comment

mikemhenry commented Apr 20, 2023

ijpulidos commented Apr 20, 2023

mikemhenry Apr 21, 2023

Choose a reason for hiding this comment

zhang-ivy commented Apr 21, 2023

ijpulidos commented Apr 23, 2023

jchodera left a comment

Choose a reason for hiding this comment

ijpulidos commented Apr 27, 2023

ijpulidos commented Apr 27, 2023

zhang-ivy commented Jun 30, 2022 •

edited

Loading

zhang-ivy commented Jul 2, 2022 •

edited

Loading

ijpulidos commented Aug 15, 2022 •

edited

Loading

codecov bot commented Aug 15, 2022 •

edited

Loading