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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

install latest openfe and gufe as a canary to catch upstream changes #204

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mikemhenry
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0d59001) 82.74% compared to head (95415ea) 82.74%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #204   +/-   ##
=======================================
  Coverage   82.74%   82.74%           
=======================================
  Files          22       22           
  Lines        2735     2735           
=======================================
  Hits         2263     2263           
  Misses        472      472           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dotsdl dotsdl left a comment

Choose a reason for hiding this comment

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

Looks great @mikemhenry! Sorry for the delay in reviewing this one! Feel free to merge if satisfied!

@dotsdl
Copy link
Member

dotsdl commented Dec 4, 2023

Actually, think this runs into the same problem that prompted #203: installing new openfe dependencies, such as kartograff in this case, that aren't required by the most recent release.

I don't know of a good workaround for this.

@@ -42,6 +47,9 @@ jobs:
environment-file: devtools/conda-envs/test.yml
activate-environment: alchemiscale-test

- name: "Install openfe & gufe from latest source"
if: ${{ matrix.deps-versions == 'openfe-latest' }}
run: pip install --no-deps git+https://github.com/OpenFreeEnergy/gufe git+https://github.com/OpenFreeEnergy/openfe
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
run: pip install --no-deps git+https://github.com/OpenFreeEnergy/gufe git+https://github.com/OpenFreeEnergy/openfe
run: |
pip install --no-deps git+https://github.com/OpenFreeEnergy/gufe git+https://github.com/OpenFreeEnergy/openfe
mamba install kartograff

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should fix the issue highlighted here #204 (comment)
We have to manually install packages to make the canary pass, that way we can "catch" the upstream change. We can also not do that and just have the CI fail until a new openfe releases comes out and fixes the dependency issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants