CICD Improvements #101

harelwa · 2022-02-24T23:30:46Z

About

this is a work in progress; it aims to check feasibility for further discussion

Following #93, further improve CI + CD

Tasks

inspiration credit https://github.com/samuelcolvin/pydantic/blob/master/.github/workflows/ci.yml

harelwa · 2022-02-25T00:34:32Z

i love coverage and observability, what can I do

hera has 95% coverage now, which is great.

Unfortunately, it's not observable in CI and.or for developers and maintainers. So I gave a try of adding a report to CI...

To create a proper report, one needs to combine the coverage reports created by all py versions builds tests.

As you can see if you look inside -- this fails .. why ?

Astox runs the tests in isolated envs, with installation of hera, coverage runs on this installation ..
thus a report can't be created, as in the coverage-report gh-wf job these tox envs never existed.

uploading and downloading all these envs as artifacts is not an option afaic; they are too big

while this is annoying, it took me again to luigi to see how they dealt with it.

luigi uses codecov uploader to "solve this" but actually get much more from what I can see

note they are using it in a way that will soon be deprecated, so the reference is relevant only in part

codecov is free for public projects, and from what I can tell can integrate easily, "solves" this issue, and provide with some quite cool visibility features built into github -

https://docs.codecov.com/docs/github-checks

@flaviuvadan - I'd like to give codecove a try. what do you say ?

https://github.com/apps/codecov

harelwa · 2022-02-25T01:00:04Z

@flaviuvadan - for some reason I can't mention https://github.com/hirosassa ; can you please add him to the discussion ?

flaviuvadan · 2022-02-25T15:14:33Z

@flaviuvadan - for some reason I can't mention https://github.com/hirosassa ; can you please add him to the discussion ?

@hirosassa 🙂

hirosassa · 2022-02-25T21:30:34Z

hera has 95% coverage now, which is great.

Oh, this is great!

I'll take a look about failure CI.

harelwa · 2022-03-07T23:29:01Z

hi @flaviuvadan @hirosassa

sorry for the previous "spam"; i've deleted the questions that became irrelevant

(1) coverage combine fixed

I've added a solution for the coverage combine issue. it took a bit longer then expected; eventually i used toxs own tox.ini as a ref - https://github.com/tox-dev/tox/blob/master/tox.ini which did the trick. you can see the result in -

https://github.com/argoproj-labs/hera-workflows/runs/5456858785?check_suite_focus=true

(2) next tasks and wrap up

let's agree on these

I'd like to consolidate hera's CICD into a single workflow file, for purposes of artifacts re-use, eliminating the current duplication of test/build using job dependencies etc.

I've already added a publish job, which currently only downloads the build artifacts ( wheels ) and lists them -

https://github.com/argoproj-labs/hera-workflows/runs/5456863640?check_suite_focus=true

These artifacts can be uploaded to pypi "as is" without first uploading to pypi test like the current CD.
This is thanks to tox which tests those build via install in (so-called) isolated envs.

Such a change will also require a -

(2.1) if condition

  publish:
    needs: [coverage]
    runs-on: ubuntu-latest
    if: "success() && (startsWith(github.ref, 'refs/tags/') || github.ref == 'refs/heads/master')"

(2.2) change of the workflow event triggers

on:
  push:
    branches:
      - main
  pull_request: {}

What say ya ?

hirosassa

Thanks for the great implementation! I added a few comments.

hirosassa · 2022-03-08T08:58:55Z

setup.cfg

+[coverage:run]
+relative_files = True
+source =
+    src/hera


@harelwa As we discussed following thread about the configuration file,
#93 (comment)

I disagree with putting the configuration of coverage and pytest separately (both are test related configs).
I prefer putting the both configuration on setup.cfg or tox.ini.

yep. and tox.ini indeed has pytest and coverage configuration. this is a left over. thanks for pointing it out. removed.

this deletion actually fixed the combine ! the combined result before the deletion was 0% 😖 . thank you @hirosassa for catching this

setup.py

Co-authored-by: hirosassa <[email protected]>

flaviuvadan · 2022-03-11T13:49:03Z

.github/workflows/hera_pr.yaml

@@ -3,7 +3,7 @@ name: Hera PR build
 on: pull_request

 jobs:
-  hera-pr-build:


Is the job name representative of the distribution that is going to be built? 🤔

I'm not sure I understand - by distribution you mean linux ?

Is this question maybe related to #101 (comment) ?

Yes, thank you for clarifying! 🙂

flaviuvadan · 2022-03-11T13:50:09Z

.github/workflows/hera_pr.yaml

+      - name: upload build artifacts
+        uses: actions/upload-artifact@v2
+        with:
+          name: pypi-build-arts
+          path: .tox/dist


This seems to publish the constructed distributions as artifacts. To be clear, these are different than the actually published package, correct?

Not quite. If we consolidate to a single GH workflow like I suggested, the build ( and test ) job will run as a dependency of publish job on push to main as well ( see more in #101 (comment) ), and thus provide us with the necessary Distribution builds ( wheels ) - As you can see Distribution builds added to setup.py are OS + Py version specific.

HOWEVER ! what's usually done, is building also a py-none-any wheel + tar.gz and publishing it as well ( like hera publishes now ), e.g. ( from https://pypi.org/project/pydantic/ )

flaviuvadan · 2022-03-11T13:51:06Z

.github/workflows/hera_pr.yaml

+      - name: upload coverage files
+        uses: actions/upload-artifact@v2
+        with:
+          name: coverage-data
+          path: ".tox/.coverage.*"


Does the publication of coverage files allow one to construct a test coverage GitHub badge? I am a bit unfamiliar with the necessary setup of coverage so I am very excited this is added, and I would love to see the report on the main repo README! I must say a direct thank you here, as a comment, for the efforts 🚀

With pleasure.

Yes, we are definitely going for a n honorable shield.

See coverage in #101 (comment)

flaviuvadan · 2022-03-11T13:52:04Z

.github/workflows/hera_pr.yaml

+      - name: get pypi build arts
+        uses: actions/download-artifact@v2
+        with:
+          name: pypi-build-arts
+          path: dist
+
+      - name: list pypi build arts
+        run: |
+          ls -la dist/
+
+      - name: install twine
+        run: |
+          python -m pip install --upgrade twine
+
+      - name: Check build
+        run: |
+          twine check dist/*


This is a check of the built distribution published via the PR, correct?

EDITED

Yes. It's not published though. It's uploaded, and then downloaded in publish job, where it will be published if event is push to main ( change not pushed yet ) this is related to #101 (comment)

flaviuvadan · 2022-03-11T13:52:27Z

This looks awesome 🙂 I left some questions mostly to clarify my own understanding of the contribution.

flaviuvadan · 2022-03-11T13:55:55Z

I still think there's a lot of value in first publishing to the Test PyPI as it ensures the whole installation and usage workflow works as expected. I am open to be convinced of this not being the case, though. I recall an issue in which a broken Hera package was published - the publication step and tests were successful because of accessibility to Hera via the editable mode, but the installed package from PyPI was broken because of some setup.py changes. Any thoughts @harelwa and @hirosassa?

flaviuvadan · 2022-03-11T13:59:18Z

Oh, another question if I may - does the consolidation in a single GHA workflow file support differentiation between PRs and main pushes to the degree that is necessary in order to support publication of PR artifacts, such as coverage, and then publish the Hera package independently when the PR is merged?

flaviuvadan · 2022-03-11T14:01:15Z

@hirosassa do you think we should merge this first and then #94, maybe? I wonder what's the best order and mix of PRs that minimizes the effort you and @harelwa need to put in to merge these (due to conflicts).

harelwa · 2022-03-11T14:34:04Z

Hi @flaviuvadan

I still think there's a lot of value in first publishing to the Test PyPI as it ensures the whole installation and usage workflow works as expected. I am open to be convinced of this not being the case, though. I recall an issue in which a broken Hera package was published - the publication step and tests were successful because of accessibility to Hera via the editable mode, but the installed package from PyPI was broken because of some setup.py changes. Any thoughts @harelwa and @hirosassa?

If you recall, I was the one to point the publish issue #51 where I also suggested a solution #51 (comment) and ended up hopefully contributing the suggestion in this PR !

I'll try and explain why publishing to Test PyPi is an overhead with the CI flows suggested in this PR -

As we are using tox now, the actual wheels that are going to be published (90% true; I'll explain more below) are installed and tested thus you do not expect pitfalls like we saw in #51 usually involving using pytest on sources, rather on build due to PyTest PYTHONPATH Fun. we also twine check explicitly to verify pypi artifacts health.

having said that - of course we can add such a job / step if you are still not comfortable with that; it will do no harm, of course.

What I did want to to use Test PyPi is to "test" the real publish flow in single GH workflow in this PR ( more below; it's not pushed yet )

Oh, another question if I may - does the consolidation in a single GHA workflow file support differentiation between PRs and main pushes to the degree that is necessary in order to support publication of PR artifacts, such as coverage, and then publish the Hera package independently when the PR is merged?

(1) coverage

If we wish to add a coverage shield ( and we do :), we can have such "external" uploads taken care of in a different work flow, but actually also in the same one.

There are a few options for the coverage shield _hosting that I've come across -

luigi use codecove; see https://github.com/spotify/luigi/blob/master/.github/workflows/pythonbuild.yml

pydantic uses smokeshow; see https://github.com/samuelcolvin/pydantic/blob/master/.github/workflows/upload-previews.yml

(2) publishing

Indeed. I provided a snippet for how it's done in #101 (comment) (2.1) + (2.2)

refs

@hirosassa do you think we should merge this first and then #94, maybe? I wonder what's the best order and mix of PRs that minimizes the effort you and @harelwa need to put in to merge these (due to conflicts).

I tend to agree it would be wiser to merge this PR first

harelwa · 2022-03-18T14:10:05Z

Hi @flaviuvadan + @hirosassa

I think we are pretty much done here.

Here are the final leftovers:

delete file .github/workflows/hera_build_and_publish.yaml
rename file .github/workflows/hera_pr.yaml -> .github/workflows/cicd.yaml ( or .. )
fix jobs badge workflow name hera_build_and_publish.yaml -> cicd.yaml ( or .. ) in README.md
remove the || true from publish job if: ( added so publish runs as part of this PR for wf validation )
enable upload to pypi-test for the purpose of validating entire publish job ( see publish job failure )
replace upload to pypi-test with upload to pypi in publish job
setup codecov if we want a coverage badge to be introduced in this PR ( or propose a different solution )
question do you want to add windows-latest to the os matrix ?

Thanks,

Harel

hirosassa

Looks good to me! Thanks for your great contribution.

question do you want to add windows-latest to the os matrix ?

I think it is unnecessary because Argo and Hera is not running on Windows and Windows has WSL currently, IMO.

hirosassa · 2022-03-18T20:22:46Z

.github/workflows/hera_pr.yaml

+      - name: upload coverage files
+        uses: actions/upload-artifact@v2
+        with:
+          name: coverage-data
+          path: ".tox/.coverage.*"


README.md

Co-authored-by: hirosassa <[email protected]>

harelwa · 2022-03-25T10:04:35Z

Hi @flaviuvadan + @hirosassa

I think we are pretty much done here.

Here are the final leftovers:

delete file .github/workflows/hera_build_and_publish.yaml

rename file .github/workflows/hera_pr.yaml -> .github/workflows/cicd.yaml ( or .. )

fix jobs badge workflow name hera_build_and_publish.yaml -> cicd.yaml ( or .. ) in README.md

remove the || true from publish job if: ( added so publish runs as part of this PR for wf validation )

enable upload to pypi-test for the purpose of validating entire publish job ( see publish job failure )

replace upload to pypi-test with upload to pypi in publish job

setup codecov if we want a coverage badge to be introduced in this PR ( or propose a different solution )

question do you want to add windows-latest to the os matrix ?

Thanks,

Harel

Hi @flaviuvadan -

Handing this over to you as all remaining tasks require maintainer permissions, access to secrets etc.

As for the merge process to main, here are my 3 cents --

Integrate as is -> [ it will ] publish to pypi-test -> validate upload manually -> change to pypi official

Harel

flaviuvadan · 2022-03-27T17:21:41Z

Thank you for the efforts @harelwa 🙂 I was on vacation for a week and am just getting back! I am going to merge this PR today and add the necessary secrets.

harelwa · 2022-03-27T18:06:40Z

Hi @flaviuvadan

I see the unfortunate failures.

Following -

https://stackoverflow.com/questions/59451069/binary-wheel-cant-be-uploaded-on-pypi-using-twine

https://peps.python.org/pep-0513/#rationale

I see auditwheel can fix that.

I'll give it a try

flaviuvadan · 2022-03-27T18:16:57Z

Hi @flaviuvadan

I see the unfortunate failures.

Following -

https://stackoverflow.com/questions/59451069/binary-wheel-cant-be-uploaded-on-pypi-using-twine

https://peps.python.org/pep-0513/#rationale

I see auditwheel can fix that.

I'll give it a try

Thank you for being on top of this, @harelwa! I started reading about manylinux and cibuildwheel myself to diagnose the issue and think about a fix.

harelwa · 2022-03-27T18:46:24Z

Hi @flaviuvadan
I see the unfortunate failures.
Following -
https://stackoverflow.com/questions/59451069/binary-wheel-cant-be-uploaded-on-pypi-using-twine
https://peps.python.org/pep-0513/#rationale
I see auditwheel can fix that.
I'll give it a try

Thank you for being on top of this, @harelwa! I started reading about manylinux and cibuildwheel myself to diagnose the issue and think about a fix.

Hi @flaviuvadan -

The immediate fix with minimal changes is to not publish the platform wheels.

You can achieve this by removing / commenting out the upload step in test job -

https://github.com/argoproj-labs/hera-workflows/blob/main/.github/workflows/cicd.yaml#L60

Then only none-any will be published ( as did thus far ).

As for repairs etc. -- perhaps for later --

When trying to repair with auditwheel I get -

>$ auditwheel repair ./tox/dist/hera_workflows-2.2.1-cp39-cp39-linux_x86_64.whl 
INFO:auditwheel.main_repair:Repairing hera_workflows-2.2.1-cp39-cp39-linux_x86_64.whl
INFO:auditwheel.main_repair:This does not look like a platform wheel

as for the discussion in pypa/auditwheel#247 -- a bit odd to me as libc et. al. issues are a matter of python it self, not only compiled code in the package...

From what I can see cibuildwheel can also be used; note it requires docker

What do you think ?

flaviuvadan · 2022-03-27T18:50:27Z

You can achieve this by removing / commenting out the upload step in test job -

I agree with the proposed approach to comment out the upload steps for now! Let's do that, and then investigate how we can introduce cibuildwheel as a package to build wheels with.

harelwa · 2022-03-27T19:01:38Z

You can achieve this by removing / commenting out the upload step in test job -

I agree with the proposed approach to comment out the upload steps for now! Let's do that, and then investigate how we can introduce cibuildwheel as a package to build wheels with.

great. will you take care of it or you'd rather have me open a PR ?

JacobHayes · 2022-03-27T22:09:37Z

Hi, apologies for jumping in! Does hera have any compiled extensions in the wheel? If not, I think the stock py3-none-any wheel will cover all platforms automatically and we can skip any cibuildwheel stuff. Quoting from the setuptools docs:

Platform Wheels are wheels that are specific to a certain platform like Linux, macOS, or Windows, usually due to containing compiled extensions.

The wheel package will detect that the code is not pure Python, and build a wheel that’s named such that it’s only usable on the platform that it was built on. For details on the naming of wheel files, see PEP 425.

And testing a bit, everything looks good:

$ sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows # Apple M1
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
$ docker run --rm -it --platform=linux/x86-64 python:3.7 sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
$ docker run --rm -it --platform=linux/x86-64 python:3.9 sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
$ docker run --rm -it --platform=linux/amd64 python:3.7 sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
$ docker run --rm -it --platform=linux/amd64 python:3.9 sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)

--

I think this might also explain what @harelwa saw with:

>$ auditwheel repair ./tox/dist/hera_workflows-2.2.1-cp39-cp39-linux_x86_64.whl 
INFO:auditwheel.main_repair:Repairing hera_workflows-2.2.1-cp39-cp39-linux_x86_64.whl
INFO:auditwheel.main_repair:This does not look like a platform wheel

-> there is no compiled code?

harelwa · 2022-03-28T05:53:59Z

Hi, apologies for jumping in! Does hera have any compiled extensions in the wheel? If not, I think the stock py3-none-any wheel will cover all platforms automatically and we can skip any cibuildwheel stuff. Quoting from the setuptools docs:

Platform Wheels are wheels that are specific to a certain platform like Linux, macOS, or Windows, usually due to containing compiled extensions.
The wheel package will detect that the code is not pure Python, and build a wheel that’s named such that it’s only usable on the platform that it was built on. For details on the naming of wheel files, see PEP 425.

And testing a bit, everything looks good:
$ sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows # Apple M1
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
$ docker run --rm -it --platform=linux/x86-64 python:3.7 sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
$ docker run --rm -it --platform=linux/x86-64 python:3.9 sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
$ docker run --rm -it --platform=linux/amd64 python:3.7 sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
$ docker run --rm -it --platform=linux/amd64 python:3.9 sh -c 'pip3 install wheel && pip3 install hera-workflows' | grep hera_workflows
  Downloading hera_workflows-2.2.1-py3-none-any.whl (41 kB)
--

I think this might also explain what @harelwa saw with:
>$ auditwheel repair ./tox/dist/hera_workflows-2.2.1-cp39-cp39-linux_x86_64.whl 
INFO:auditwheel.main_repair:Repairing hera_workflows-2.2.1-cp39-cp39-linux_x86_64.whl
INFO:auditwheel.main_repair:This does not look like a platform wheel
-> there is no compiled code?

Thank you @JacobHayes

Yes. I agree. I think we can drop the platform wheels all together.

harelwa added 4 commits February 25, 2022 00:45

add optional binary distribution to setup.py

71f12ff

move pytest configuration to tox.ini

a106b2f

add coverage report task to makefile

ae59a2c

add coverage and publish jobs to gh-workflow

701eb0c

add conda-forge shield to README

8a9e383

fix coverage combine

415475a

harelwa changed the title ~~WIP: CICD Improvements~~ CICD Improvements Mar 7, 2022

harelwa added 2 commits March 8, 2022 00:08

add more docs

82a73f1

fix workflow

891836a

remove coverage combine --keep

351a33a

hirosassa suggested changes Mar 8, 2022

View reviewed changes

harelwa and others added 2 commits March 8, 2022 11:08

remove converage configuration from setup.cfg

756db3d

Update setup.py

61b12bc

Co-authored-by: hirosassa <[email protected]>

flaviuvadan reviewed Mar 11, 2022

View reviewed changes

fix tox usage example

1c5589c

harelwa force-pushed the cicd-improvements branch from 1f29b5a to d41824f Compare March 11, 2022 15:53

add os to build matrix with linux, macos, windows

7259d93

consolidate cicd ghwf

f0122aa

harelwa force-pushed the cicd-improvements branch from 409c72f to f0122aa Compare March 18, 2022 11:02

harelwa added 2 commits March 18, 2022 13:25

revert to dotted suffix coverage files

235860c

test publish conditions

a14f862

harelwa force-pushed the cicd-improvements branch from d9087ff to a14f862 Compare March 18, 2022 13:37

tox: faster lint task ( skip_install = true )

4818c3e

wf cosmetics

170b229

harelwa force-pushed the cicd-improvements branch from 299b492 to 170b229 Compare March 18, 2022 14:20

harelwa and others added 3 commits March 18, 2022 16:30

docs fix

4f28168

remove prev publish workflow

d34c4b7

Merge branch 'argoproj-labs:main' into cicd-improvements

fb1b835

hirosassa approved these changes Mar 21, 2022

View reviewed changes

harelwa and others added 2 commits March 21, 2022 15:51

Update README.md

950af97

Co-authored-by: hirosassa <[email protected]>

refactor workflow file name

b47ade3

flaviuvadan added 2 commits March 27, 2022 13:33

Publish to official PyPI index

0014633

Format

f299da7

flaviuvadan approved these changes Mar 27, 2022

View reviewed changes

flaviuvadan merged commit c4d2530 into argoproj-labs:main Mar 27, 2022

flaviuvadan mentioned this pull request Mar 27, 2022

Remove artifact uploads #110

Merged

flaviuvadan mentioned this pull request Oct 28, 2022

Suggestion: Set up release automation and changelog #334

Closed

CICD Improvements #101

CICD Improvements #101

Conversation

harelwa commented Feb 24, 2022 • edited by flaviuvadan Loading

About

Tasks

harelwa commented Feb 25, 2022 • edited Loading

harelwa commented Feb 25, 2022

flaviuvadan commented Feb 25, 2022

hirosassa commented Feb 25, 2022

harelwa commented Mar 7, 2022 • edited Loading

hirosassa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harelwa Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

flaviuvadan commented Mar 11, 2022

flaviuvadan commented Mar 11, 2022

flaviuvadan commented Mar 11, 2022

flaviuvadan commented Mar 11, 2022

harelwa commented Mar 11, 2022 • edited Loading

harelwa commented Mar 18, 2022 • edited Loading

hirosassa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harelwa commented Mar 25, 2022

flaviuvadan commented Mar 27, 2022

harelwa commented Mar 27, 2022

flaviuvadan commented Mar 27, 2022

harelwa commented Mar 27, 2022 • edited Loading

flaviuvadan commented Mar 27, 2022

harelwa commented Mar 27, 2022 • edited Loading

JacobHayes commented Mar 27, 2022 • edited Loading

harelwa commented Mar 28, 2022

harelwa commented Feb 24, 2022 •

edited by flaviuvadan

Loading

harelwa commented Feb 25, 2022 •

edited

Loading

harelwa commented Mar 7, 2022 •

edited

Loading

harelwa Mar 11, 2022 •

edited

Loading

harelwa commented Mar 11, 2022 •

edited

Loading

harelwa commented Mar 18, 2022 •

edited

Loading

harelwa commented Mar 27, 2022 •

edited

Loading

harelwa commented Mar 27, 2022 •

edited

Loading

JacobHayes commented Mar 27, 2022 •

edited

Loading