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

Updating CI workflow #93

Merged
merged 76 commits into from
Aug 17, 2023
Merged

Updating CI workflow #93

merged 76 commits into from
Aug 17, 2023

Conversation

mattkwiecien
Copy link
Contributor

@mattkwiecien mattkwiecien commented Jul 28, 2023

This PR aims to expand our test coverage and unify the CI workflow with the regular development workflow. This is useful because we want the build action to reflect the development workflow that a user who is contributing to the codebase would experience. Currently the two are pretty disjointed (different installs etc).

  • Combined lint and test into one single ci.yaml file
  • Expanded our CI to test on both macOS and Ubuntu, and testing both 3.8 and 3.11. We can test in between versions, but I think that may be overkill

The new workflow is lint > tests > publish

  • Lint
    • Replaced manual install of flake8, black, and coveralls with their associated published github action
  • Tests
    • Using mamba for speed and installing from our new environment.yml file
    • Updated environment.yml to include all development dependencies, and instructing a user to pip install with --no-deps to prevent dependency clashes. Removing defaults from channels to prevent similar issues
    • Added cache that resets daily for each mamba environment.
    • NOTE: writing to .coverage isn't thread safe, so changed our testing workflow to run ALL tests, and generate a coverage report. THEN run the mpi tests in parallel, but do not generate a coverage report from them.
    • Lastly, post to coveralls and then wait for the finish webhook to complete the coverage report
  • Publish
    • Post to coveralls.io and let them know all tests are finished.

@mattkwiecien
Copy link
Contributor Author

@carlosggarcia Ready to review, finally!

@mattkwiecien
Copy link
Contributor Author

Note that the build check will never pass as it's been changed. We will need to update this once the PR is merged.

Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

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

LGTM. Solve the conflicts and I will recheck and approve.

README.md Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
if: steps.cache.outputs.cache-hit == 'true'
run: |
mamba install -y openmpi
pip install -U mpi4py --no-cache-dir --no-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

why both mamba and pip?

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'm honestly not sure how to tell conda to build with MPICH or OpenMPI? By using --no-deps we prevent pip from installing any dependencies besides just the mpi4py module

Copy link
Contributor

Choose a reason for hiding this comment

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

whatever, if it works, let's leave it like this

Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

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

go merge!

@mattkwiecien mattkwiecien merged commit 28e16cc into master Aug 17, 2023
6 checks passed
@mattkwiecien mattkwiecien deleted the ci-cleanup branch February 15, 2024 22:21
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