-
Notifications
You must be signed in to change notification settings - Fork 103
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
Generalize forcefields for generic ASE calculator support #940
Changes from 48 commits
f05bce8
5e538e3
d861b94
786ce93
4cd538f
e25f61e
e2eb2c1
756c3fe
e32636c
5acc792
924e910
e83deaf
b0f7c7c
77a5833
e6f83a9
e951a05
1c1142d
b1952c9
4b1915b
b0c9a77
9b13e45
c57d9ff
89eb1c7
8d9a7ee
d5b3a87
ea6f1ea
625d971
9b341d2
38449da
610ac72
6adae2f
a90170f
834ce73
af0f978
be6829e
82d7dca
526b5ad
d07ca25
b243b73
3efff87
c5f7fe8
4a12353
7cf617a
73c4269
82ab00e
e4f1803
86fe898
b652a3c
e7edeea
df4eca8
348678e
0d181e1
c6d6ffc
eb51282
321a975
0d5c46a
93dd9cc
9585dc9
8d85829
bcd23f4
73caa68
dcaeb85
ff44149
902823d
e0bf592
0aa8a08
7b25b10
8f305e9
808de7d
ec3a3c5
4508b9f
0b4987e
da01791
063c1c9
ffacf71
763bcc6
aada903
be168e4
027defd
7aa7675
ed544be
c1db9e6
bb0d2e8
9b077b1
1e21c6b
362db23
a638f42
f110574
4ba239b
4c581bc
3e68d3d
00aaf8d
4c3c13c
00ea296
4b0d680
cde15a0
daf460f
a40accb
e576828
1c41666
7e5abd6
1b64286
5fe8c1f
3ea5f3e
44bc54c
0b74403
b72c91c
4d1eeae
9ca6d18
481a9c9
f29d17b
f120cfc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ jobs: | |
|
||
runs-on: ubuntu-latest | ||
strategy: | ||
fail-fast: true | ||
matrix: | ||
python-version: ["3.9", "3.10", "3.11"] | ||
|
||
|
@@ -62,8 +63,8 @@ jobs: | |
mkdir -p ~/.abinit/pseudos | ||
cp -r tests/test_data/abinit/pseudos/ONCVPSP-PBE-SR-PDv0.4 ~/.abinit/pseudos | ||
pip install .[strict,tests,abinit] | ||
pip install torch-runstats | ||
pip install --no-deps nequip==0.5.6 | ||
pip install tblite==0.3.0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I'll put this in an |
||
|
||
- name: Install pymatgen from master if triggered by pymatgen repo dispatch | ||
if: github.event_name == 'repository_dispatch' && github.event.action == 'pymatgen-ci-trigger' | ||
|
@@ -75,7 +76,49 @@ jobs: | |
- name: Test | ||
env: | ||
MP_API_KEY: ${{ secrets.MP_API_KEY }} | ||
run: pytest --cov=atomate2 --cov-report=xml | ||
run: pytest --ignore=tests/forcefields --cov=atomate2 --cov-report=xml | ||
|
||
- uses: codecov/codecov-action@v1 | ||
if: matrix.python-version == '3.10' && github.repository == 'materialsproject/atomate2' | ||
with: | ||
token: ${{ secrets.CODECOV_TOKEN }} | ||
file: ./coverage.xml | ||
|
||
test-forcefields: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd recommend merging all tests back into a single job (which we can then split with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like anything Outside of CI, having torch installed but not loaded seems OK with Let me know what you think is best There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
oh, i forgot about that. in that case, could you verbatim copy your above comment into the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK! Split off the ase and jupyter notebook tests into separate test instances. Also set up the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
to update the file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied all the pytest split logic from pymatgen 😂 same command for the split durations test, the CI tests just take a very different amount of time in CI vs. my machine. Guessing there's MPS hardware acceleration turned on by default in the forcefields I'll play around with this a bit - maybe setting the default device to be CPU would help for getting a better CI timing estimate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the other option would be to temp change the CI run to pytest --store-durations --durations-path=DURATIONS_PATH
cat DURATIONS_PATH and then copy paste the actual test durations in CI from the log into your local durations file |
||
services: | ||
local_mongodb: | ||
image: mongo:4.0 | ||
ports: | ||
- 27017:27017 | ||
|
||
runs-on: ubuntu-latest | ||
strategy: | ||
fail-fast: true | ||
matrix: | ||
python-version: ["3.9", "3.10", "3.11"] | ||
|
||
steps: | ||
- uses: actions/checkout@v4 | ||
|
||
- uses: actions/setup-python@v5 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
|
||
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install .[strict,strict-forcefields,tests] | ||
pip install torch-runstats | ||
pip install --no-deps nequip==0.5.6 | ||
|
||
- name: Install pymatgen from master if triggered by pymatgen repo dispatch | ||
if: github.event_name == 'repository_dispatch' && github.event.action == 'pymatgen-ci-trigger' | ||
run: pip install --upgrade 'git+https://github.com/materialsproject/pymatgen@${{ github.event.client_payload.pymatgen_ref }}' | ||
|
||
- name: Test | ||
env: | ||
MP_API_KEY: ${{ secrets.MP_API_KEY }} | ||
run: pytest --cov=atomate2 --cov-report=xml tests/forcefields | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we merge the force field tests into the main test run? ideally, we'll use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way it's partitioned now is because |
||
|
||
- uses: codecov/codecov-action@v1 | ||
if: matrix.python-version == '3.10' && github.repository == 'materialsproject/atomate2' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
"""Module for Atomic Simulation Environment workflows.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC
fail-fast: true
will reduce the amount of signal you get from a CI run. e.g. if a job running on 3.9-3.12 fails on 3.9 first, you won't know if it also would have failed on 3.10, 3.11, etc. is this intended?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly added this for my ease of figuring out why tests were failing initially - reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i forgot
fail-fast: true
is actually the default