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

Remove CircleCI from CI workflows #951

Merged
merged 124 commits into from
Jun 17, 2022
Merged

Conversation

tommy-waltmann
Copy link
Collaborator

@tommy-waltmann tommy-waltmann commented Apr 29, 2022

Description

This PR will remove CircleCI from all CI workflows, converting the windows tests to use github actions. I would also like this PR to reformat the current github actions CI to be more modular, and either fix or remove code coverage.

Motivation and Context

CircleCI workflows have been blocked because there are no credits on our plan. Github Actions is a better CI deal anyway, so its time to convert freud's CI workflows.

Coverage has been broken since our move to scikit-build.

Resolves: #947 #699

There is also an open pull request #749 which attempts to make the current CI format work with windows. I think this PR should make more sweeping CI changes than that PR would provide, so once this PR gets merged, that branch can be removed.

How Has This Been Tested?

We will push commits and make sure the CI acts as we want it to. To test CI related to releases, we will need to do some more investigation of the build artifacts. I am honestly not sure how to test that the release process works without making a release.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds or improves functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation improvement (updates to user guides, docstrings, or developer docs)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have updated the documentation (if relevant).
  • I have added tests that cover my changes (if relevant).
  • All new and existing tests passed.
  • I have updated the credits.
  • I have updated the Changelog.

@tommy-waltmann tommy-waltmann self-assigned this Apr 29, 2022
@tommy-waltmann
Copy link
Collaborator Author

Bumping the ci image from 2021.11 to 2022.05 seems to cause issues when trying to lint the C++ code. Will investigate further on this later.

@tommy-waltmann
Copy link
Collaborator Author

2019.7 seems to be our oldest supported version of tbb.

@tommy-waltmann tommy-waltmann requested review from joaander and bdice June 15, 2022 18:35
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

A few suggestions -- great work.

# requires:
# - linux-python-38
steps:
- run: echo "Nothing to do."
Copy link
Member

Choose a reason for hiding this comment

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

Just delete this file if it doesn't do anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason, circleci still tries to run even if I remove the config file and it shows up on github as a failed test. The dummy makes sure we can still get the coveted green check mark.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. That just takes some fixing on the GitHub / CircleCI website side of things. Go ahead and delete it, and let me know if you want help fixing that.

@@ -1,5 +1,5 @@
ase==3.16.0
cmake==3.12.0
cmake==3.13.0
Copy link
Member

Choose a reason for hiding this comment

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

Why was this bumped?

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 was needed because of a change made in #950 (calling target_link_options). I don't remember why it didn't get caught in that PR, but I'm fixing it here.

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great improvement.

Since the new tests are passing and circleci has become troublesome I don't want to delay this merge. I do offer a few optional suggestions for improvement.

- ninja
- pip
- pip:
- -r requirements-test-compatible.txt
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this works. conda-forge provides packages with a specific ABI that is not the same as PyPI's manylinux. Prefer the conda-forge provided packages and only fall back on pip installations when absolutely necessary. In such cases, consider contributing the package to the conda-forge ecosystem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

run_tests.yml has to install the test dependencies plus tbb, but build_wheels.yml has to install the test dependencies without tbb. I could not think of any other way to have two separate sets of requirements files without writing the same pinned dependencies in two different places.

@tommy-waltmann tommy-waltmann merged commit 048caae into master Jun 17, 2022
@tommy-waltmann tommy-waltmann deleted the fix/ci-github-actions branch June 17, 2022 19:51
@tommy-waltmann tommy-waltmann added this to the v2.11.0 milestone Aug 8, 2022
@tommy-waltmann tommy-waltmann mentioned this pull request Aug 11, 2022
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark pull requests that should have benchmarks run on them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use github actions for windows CI tests
3 participants