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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

update CI to test release candidate #787

Merged
merged 7 commits into from Jan 17, 2022
Merged

Conversation

mikemhenry
Copy link
Contributor

I removed the part we commented out for the release candidate test and also added some cases to the matrix, let me know if this looks correct @jaimergp 馃槃

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #787 (4a6c7b9) into master (b5d65a6) will decrease coverage by 0.92%.
The diff coverage is n/a.

@jaimergp
Copy link
Member

LGTM!

I posted a comment in the OpenMM issue tracker with an idea on how to automate this comment/uncomment manual switch. It involves using the Anaconda CLI to compare timestamps and only act if RCs are recent enough.

We could try it here!

@mikemhenry
Copy link
Contributor Author

I will give it a try! I think what you posted should work just fine. I'll let you know when I'm ready either for you to take a look or need some more sage advice 馃槃

@jchodera jchodera added this to the 0.9.1 Bugfix release milestone May 24, 2021
@jchodera
Copy link
Member

OpenMM 7.5.2 has been released now, so unless we have fancy logic that only tests active RCs to test, we can close this!

@mikemhenry
Copy link
Contributor Author

I'm going to keep this PR open until I add in that logic :)

@mikemhenry
Copy link
Contributor Author

See openmm/openmm#3098 (comment)

@mikemhenry
Copy link
Contributor Author

Also remove testing "latest" in the build matrix since that is the same as conda-forge now. So we will be testing:

  • latest openmm release
  • openmm rc (if there is one)
  • openmm nightly

@mikemhenry
Copy link
Contributor Author

This also drops python 3.7 testing according to NEP 29
@ijpulidos ready for review!

@mikemhenry
Copy link
Contributor Author

Oh and adds openmm 7.7 to the testing matrix

Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

Nice work!!

I just have some non-blocking question about why the rc-check and test have to be done that differently compared to the nightly, 7.6 and 7.7 tests. I don't pretend to understand the whole thing, since I'm not that familiar with github actions, but if you can summarize a bit of it that would be really helpful for me to understand and also learn :). Thanks!!

Seems good to me! LGTM.


env:
OPENMM: ${{ matrix.openmm }}
OE_LICENSE: ${{ github.workspace }}/oe_license.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that I'm still not that familiar with GHA, but, shouldn't we use secrets.OE_LICENSE here? Or maybe the question is, why do we have to use github.workspace and the PATH to the license here? Just thinking if there is a risk of the license getting exposed, no more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is is just copied from how we do it in our main yaml, right? I think the difference is in using a secret that is defined on the org level instead of the repo level. That way we don't need to copy the same secret to every repo, and also that means when we need to update or rotate it, there is a singe place we need to do it.

@mikemhenry
Copy link
Contributor Author

I just have some non-blocking question about why the rc-check and test have to be done that differently compared to the nightly, 7.6 and 7.7 tests. I don't pretend to understand the whole thing, since I'm not that familiar with github actions, but if you can summarize a bit of it that would be really helpful for me to understand and also learn :). Thanks!!

So 7.6 and 7.7 we just install from conda-forge channel.
For the nightly we use the omnia-dev channel to install the nightly build.
To test a release candidate we need to install openmm from the conda-forge/label/openmm_rc channel (which is technically just a label). Now we only want to install the release candidate when there is a new one, so the rc-check looks to see if there is a new one in the last 30 days, then if there is, runs our CI that installs the RC. That way we don't just keep testing the same RC until there is a new one.

@ijpulidos
Copy link
Contributor

So 7.6 and 7.7 we just install from conda-forge channel. For the nightly we use the omnia-dev channel to install the nightly build. To test a release candidate we need to install openmm from the conda-forge/label/openmm_rc channel (which is technically just a label). Now we only want to install the release candidate when there is a new one, so the rc-check looks to see if there is a new one in the last 30 days, then if there is, runs our CI that installs the RC. That way we don't just keep testing the same RC until there is a new one.

Ah great, I get it now, thanks for explaining this to me!

@mikemhenry mikemhenry merged commit 7909e7c into master Jan 17, 2022
@mikemhenry mikemhenry deleted the feat/add-rc-openmm-to-ci branch January 17, 2022 19:38
@mikemhenry mikemhenry mentioned this pull request Jan 17, 2022
@mikemhenry mikemhenry restored the feat/add-rc-openmm-to-ci branch January 26, 2022 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high priority high tests Unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants