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

ci: try a simple ci.yaml (experiment) #272

Merged
merged 7 commits into from
Nov 20, 2023

Conversation

jandom
Copy link
Collaborator

@jandom jandom commented Nov 11, 2023

When setting up to work on this repo, I found the setup very challenging. There is no setup script, so I worked based on what the ci.yaml does – and that contained a number of curveballs. Luckily things can be simplified and everything still works!

Copy link

codecov bot commented Nov 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9ecbdea) 69.26% compared to head (333beba) 69.26%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #272   +/-   ##
=======================================
  Coverage   69.26%   69.26%           
=======================================
  Files          22       22           
  Lines        3966     3966           
  Branches      717      717           
=======================================
  Hits         2747     2747           
  Misses       1038     1038           
  Partials      181      181           

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

@jandom jandom marked this pull request as ready for review November 11, 2023 14:22
@jandom
Copy link
Collaborator Author

jandom commented Nov 15, 2023

@orbeckst this one puts a smile on my face and makes things a bit simpler, please have a look

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

To summarize that I get it: The new idea is to install the whole testing environment in one go (with the appropriate Python and GROMACS versions and pytest) and in this way

  1. have a single env.yml file that one can also use conveniently for local testing
  2. avoid the hacky install of GROMACS that forces the installation of the required python because of the weird/broken dependency resolution around PyPy python 3.9

This looks ok to me. Generally, I like splitting up tasks and debug and monitor separately. But I can get behind 1 and 2 above.

I have some small comments about naming.

The big thing is: Please go through the actions and double check that the correct versions of GROMACS and Python are actually installed. You need to look at the final mamba list to see what's actually in the build environment. Then confirm that this is all correct for the desired entries in our build matrix.

.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
@jandom jandom requested a review from orbeckst November 19, 2023 11:44
@orbeckst orbeckst self-assigned this Nov 20, 2023
@orbeckst orbeckst merged commit 463820c into Becksteinlab:main Nov 20, 2023
17 checks passed
@orbeckst
Copy link
Member

thanks!

@jandom
Copy link
Collaborator Author

jandom commented Nov 20, 2023

Thank you for the feedback 🎉

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.

2 participants