-
Notifications
You must be signed in to change notification settings - Fork 93
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
Update GHA to use Conda-Incubator #410
Conversation
Implements the Conda-Incubator to GitHub Actions instead of old activate script. Removes the older helper scripts.
LGTM. I think we can also get rid of |
I don't want to drop the other tools yet until we are certain we've pulled what we need from them for the documentation compile and upload. |
Oops, I realized a bit too late that we need setup-miniconda v2 😬 |
@@ -23,7 +23,7 @@ jobs: | |||
fail-fast: false | |||
matrix: | |||
os: [macOS-latest, ubuntu-latest, windows-latest] | |||
python-version: [3.7, 3.8] | |||
python-version: [3.8, 3.9] |
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.
conda-forge is still building for 3.6 through 3.9---should we test with the same versions?
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.
Good question. I don't think we need to though. We want to make sure we are supporting the latest versions and following the protocol for "Most recent 2 Python Versions." I can understand supporting the "Most Recent 2"-1 version (3.7) in this case while everyone else gets pulled up.
We can specify in the meta.yaml
on conda-forge to restrict to Python >= 3.8 and it just wont build the lesser versions.
We also do not actually do anything which is Python 3.6 incompatible, I think, so there's no harm yet (not like we have any Walrus in the code yet).
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.
Maybe we don't need the full matrix for all Python versions but can still include Ubuntu. Also there's a chance pymbar
v4 goes noarch
if the C extension is dropped, so conda-forge would be simplified.
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 like having the full matrix in our own CI testing since it lets us see upstream dependency issues. But I agree, if we can go noarch
then the conda-forge build will be much simpler.
conda info | ||
python devtools/scripts/create_conda_env.py -n=test -p=$PYVER devtools/conda-envs/test_env.yaml | ||
- name: Setup Conda | ||
uses: conda-incubator/setup-miniconda@v1 |
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.
The OpenFF folks were just talking about v2
update here: openforcefield/status#13
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 bumped that in #411 since I missed it in this one.
Implements the Conda-Incubator to GitHub Actions instead of old
activate script. Removes the older helper scripts.
Partial resolution to #409