logsdail / carmm

Scripts for creation, manipulation and analysis of geometric and electronic structure of molecular models
GNU General Public License v3.0
5 stars 17 forks source link

Bringing in MACE-MP workflows to CARMM #138

Closed ikowalec closed 7 months ago

ikowalec commented 7 months ago

Now removed the [WIP] tag.

TODOs:

Done:

ikowalec commented 7 months ago

The CI tests will likely fail due to missing imports. Rebuilding torch as part of CI is not an option due to size - @GabrielBram ideas?

Edit: for now using a dry_run flag and EMT() calculator

codecov[bot] commented 7 months ago

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (68fddf9) 86.58% compared to head (672132b) 87.50%.

Files Patch % Lines
carmm/run/workflows/react_mace.py 87.39% 15 Missing :warning:
carmm/run/workflows/helper.py 71.42% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #138 +/- ## ========================================== + Coverage 86.58% 87.50% +0.92% ========================================== Files 72 74 +2 Lines 2847 3033 +186 ========================================== + Hits 2465 2654 +189 + Misses 382 379 -3 ``` | [Flag](https://app.codecov.io/gh/logsdail/carmm/pull/138/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Logsdail) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/logsdail/carmm/pull/138/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Logsdail) | `87.50% <90.10%> (+0.92%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Logsdail#carryforward-flags-in-the-pull-request-comment) to find out more.

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

GabrielBram commented 7 months ago

Look through the MACE ASE calculator, they unfortunately import torch at the head so I don't believe there's a way around this. Would this require Andy to update the CI test runner?

ikowalec commented 7 months ago

Look through the MACE ASE calculator, they unfortunately import torch at the head so I don't believe there's a way around this. Would this require Andy to update the CI test runner?

We would have to cache the pytorch installation somehow - we can't just download and rebuild 2.5gb of packages on each pull request. Something to think about.

Edit: Apparently there is a cached version of pytorch, so dependancies are building only ~60 sec longer with torch, torchvision and mace in the CI test

ikowalec commented 7 months ago

Look through the MACE ASE calculator, they unfortunately import torch at the head so I don't believe there's a way around this. Would this require Andy to update the CI test runner?

now resolved by amending the main.yml