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

[FEATURE]: Revamp/Modularize CI #716

Closed
davidegraff opened this issue Mar 8, 2024 · 2 comments · Fixed by #714
Closed

[FEATURE]: Revamp/Modularize CI #716

davidegraff opened this issue Mar 8, 2024 · 2 comments · Fixed by #714
Assignees
Labels
enhancement a new feature request
Milestone

Comments

@davidegraff
Copy link
Contributor

Summary
This is an extension of discussion started on #710 is currently being addressed by #714. I'm opening this issue to centralize the discussion.

Proposal
From here, I think our optimal CI/CD should have the following jobs:

  • build: check that our code is free of syntactical errors
  • test: "..." passes all of our unit tests
  • lint: "..." passes our internal code formatting (black, autopep8, etc.)
  • publish: push a new release to PyPI (and Anaconda potentially)
  • deploy: push the current most recent release to Dockerhub
  • docs: rebuild and deploy the docs of our most recent version

I draw a distinction between "release" and "version" here, where a release as something that has been tagged and a version is synonymous with a commit (because each commit can be version tagged compliant with PEP440). So mechanically, the only difference between a release and a version is that we've chosen to tag a given commit.

My current view is that the dependency DAG should look like this:

┌───────────────────┐   
│build              │   
└┬───────────┬─────┬┘   
┌▽─────────┐┌▽───┐┌▽───┐
│test      ││lint││docs│
└┬────────┬┘└────┘└────┘
┌▽──────┐┌▽─────┐       
│publish││deploy│       
└───────┘└──────┘       

Note: The DAG is also missing triggers (i.e., on-events)

The logic:

  • build is the base of the hierarchy. If code is not syntatically valid, it can't be parsed (lint//docs) and definitely can't be run test)
  • lint has no downstream dependencies. Failing lint is something to be concerned about, but it is not fatal
  • test is the base of our release workflows: publish and deploy. If our code fails testing, then we should publish a release.
    question: Is it possible to have logic that rejects a "release" (e.g. a tag) if it fails tests?
  • docs has no dependency other than build. If somehow we push a commit that fails tests, we shouldn't suddenly not up-to-date documentation.

I have limited experience in using GH actions, but it would be nice if we could somehow define these all to be independent units of work from which we can compose specific actions for each event:

  • commit
  • PR
  • cron
  • release
@davidegraff davidegraff added the enhancement a new feature request label Mar 8, 2024
@kevingreenman kevingreenman added this to the v2.0.0 milestone Mar 11, 2024
@kevingreenman kevingreenman linked a pull request Mar 13, 2024 that will close this issue
@JacksonBurns
Copy link
Member

#714 has a close approximation of this, with some differences I think are for the better. We first execute a build which tries to run the build package (which we use for distribution). If that fails we stop, if that passes we then run the linter. These two steps finish very fast (<1 min) the idea being that changes can quickly be screened for these 'basic' checks and acted on before doing the tests.

We then run the tests and of course fail if the tests do not pass.

There is a separate action which will build and push packages to PyPI when a version tag is pushed to main. While not literally tied to the CI passing, we theoretically won't push anything to main that doesn't pass the CI so it is effectively tied to the CI passing.

I will note that this CI setup does require the previous steps to pass before running any of the next ones. I think we should enforce this since we want all code to match all the standard, but I'm open to input here.

The conda build is handled externally in the feedstock repo, and the docs build is done... somewhere? @kevingreenman is the docs build handled by readthedocs?

@kevingreenman
Copy link
Member

@JacksonBurns yes, the RTD build is done by RTD based on configurations/instructions in https://github.com/chemprop/chemprop/blob/v2/dev/docs/source/conf.py and https://github.com/chemprop/chemprop/blob/v2/dev/.readthedocs.yml

@KnathanM KnathanM closed this as completed Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a new feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants