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] Use concurrency to cancel-in-progress workflows on new commit #427

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

psobolewskiPhD
Copy link
Contributor

This PR adds concurrency and cancel-in-progress to the CI workflows. See:
https://docs.github.com/en/actions/using-jobs/using-concurrency
What this means is if workflows are running when a new commit is pushed, they are canceled and restarted, rather than piling up.

I tested this on my fork, see: https://github.com/psobolewskiPhD/numcodecs/actions/runs/4600373158

TODO:

  • Unit tests and/or doctests in docstrings
  • Tests pass locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • Docs build locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Codecov passes)

@psobolewskiPhD
Copy link
Contributor Author

BTW, looking at all the actions, it occurs to me that every commit gets tested twice?
Once in the OS_CI action, e.g. ci-osx:

- name: Run tests
shell: "bash -l {0}"
run: |
conda activate env
pytest -v

But also when the wheel is built:
CIBW_TEST_COMMAND: pytest --pyargs numcodecs

Maybe the wheels action should use a simpler test command, like:
python -c 'import numcodecs'

@joshmoore
Copy link
Member

Thanks again, @psobolewskiPhD! (Workflows launched)

Maybe the wheels action should use a simpler test command, like:

MSTM.

@psobolewskiPhD
Copy link
Contributor Author

Not familiar with MSTM 😬
Any help?

@joshmoore
Copy link
Member

Sorry, "makes sense to me" :)

@psobolewskiPhD
Copy link
Contributor Author

Sorry, "makes sense to me" :)

thanks 😅
Do you think it makes sense to:

  1. add to this PR—it's tangentially relevant?
  2. Make a new PR for that?
  3. Add it to the PR with arm64 wheels, so we see the times better?

@psobolewskiPhD
Copy link
Contributor Author

Gentle bump—I should have some time to return to this topic.

@joshmoore
Copy link
Member

Hey, @psobolewskiPhD. Thanks for the bump. Very appreciated.

Do you think it makes sense to:

  • add to this PR—it's tangentially relevant?
  • Make a new PR for that?
  • Add it to the PR with arm64 wheels, so we see the times better?

Let's get this PR in. If you think we can fast-lane a new PR, let's do that. Otherwise, use the arm64 PR.

If you would like to see this PR listed in the release notes, please feel free to add it in one of your (many) other PRs. 😄

@joshmoore joshmoore merged commit 948da6d into zarr-developers:main Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants