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

Cleaning and improvements in the Build Pipeline #2873

Merged
merged 63 commits into from
Aug 30, 2024

Conversation

mquinnfd
Copy link
Contributor

@mquinnfd mquinnfd commented Aug 26, 2024

Overview:

Hey it's the draft PR guy again!

This is a WIP branch where I'm attempting to clean up the build pipeline as per #2857 - which is to say, implementing the hygiene that was omitted from the first iteration of the "get poetry working" work

The goal of this work is not only jank reduction, but:

  • Build the package(s) once and once only
  • Ship what we test
  • Use caching and concurrency to speed things up as much as possible
  • Make the pipeline easier to read and change

For @cyberw @andrewbaldwin44 and community

Major

  • Updated build pipeline
    • Create distinct build, test and publish phases
    • Build packaged artifacts once, and use these in test and publishing phases
    • Added CI Docker image which picks up the built package instead of rebuilding it
    • Attempts to consolidate publishing steps
    • Attempts to consolidate generation of build metadata
  • Added poe to handle pre-builds
    • Eliminated renaming of wheels with multi-arch jank, and use of undocumented poetry feature
  • Added a rudimentary test of the docker image
  • Updated tox steps for integration testing to allow this to use an externally-built artifact
  • Added the poetry.lock file to start caching dependency versions, but this could be reverted, there are tradeoffs I'll outline in the comments

Minor

  • Adds caching for NPM steps
  • Brings back integration testing on MacOS
  • Updated developing locust docs to be less specific about how to install poetry
  • Added tox test to make sure the docs build works
  • Updated comment on the pre-commit to point to the right ruff version marker
  • Allowed dist to be picked up by the Docker context for the ci image

@mquinnfd
Copy link
Contributor Author

mquinnfd commented Aug 26, 2024

FYI this is mostly for consideration in the open, branch commits are dirty as I've been running remote builds a lot.

Things on my mind:

  • I had split out the building, testing and packaging of the Docker image, but ended up consolidating this because multi-arch builds can't be easily exported and passed around with buildx
  • I havn't adequately tested the publishing steps, to do this I'm probably going to make my own PyPi dummy repo and credentials, same with Dockerhub
  • Maybe we could have one Docker image, and have a flag in the build steps somewhere that picks up the dist or rebuilds the code, but this might be messier
  • I could probably shave a tiny amount of time from the critical path by splitting the tox matrix into 2 matrices, but the additional complexity might not be worth it
  • I need to get the tox unit tests using the venv cache as it seems to miss these sometimes
  • I tried to simplify the specifics of the "on master" vs "tag build" vs "merge commit" and all that, but the logic needs a sense check to make sure it's doing what we want

@mquinnfd
Copy link
Contributor Author

mquinnfd commented Aug 26, 2024

On the subject of committing back the poetry.lock file:

It's a tradeoff. Keeping it will mean running a poetry lock now and then to make sure it gets updated along with changing versions of non-hard-pinned dependencies. It's good for reproducibility, but then I've been adding this to the built dist anyway despite not having it in source control. It's somewhat helpful to aid in caching as well, as we can invalidate build caches when the lock file changes (as with NPM etc.)


There are some useful parallels with Rust's cargo as mentioned in the below thread

keeping the lockfile locally is a trade-off: it's true that you get reproducibility, but it also means that if some new dependency has been released that breaks things for consumers of your library, you're likely to be the last to find out about it.

python-poetry/poetry#7488
https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control

@mquinnfd mquinnfd changed the title Build pipeline cleanup merge Cleaning and improvements in the Build Pipeline Aug 26, 2024
@mquinnfd
Copy link
Contributor Author

Subsequent builds will be quicker due to the addition of caching, I may re-run the actions for this ref for this reason, also looks like we got a slow runner.

@mquinnfd
Copy link
Contributor Author

Working on CI pipelines be like

image

@cyberw
Copy link
Collaborator

cyberw commented Aug 27, 2024

Looking good! Let me know when it is ready for for final review!

@mquinnfd
Copy link
Contributor Author

Looking good! Let me know when it is ready for for final review!

👍 no worries, I really just need to test the publication logic, which is hard to do without standing up some PyPi/Docker repo, so I'll do this as soon as I can to test it out on my fork then I'll flag this for review

@mquinnfd
Copy link
Contributor Author

So I tested out the publishing logic and fixed a few quirks to do with tag resolution etc.
I now have some test projects set up that I can push to, will delete these repos once merged of course

Artifacts have been published successfully to:

Would consider this in a review-able state:

  • It's certainly better than what's there currently
  • Main areas to check will be expected behaviour on merge, tag and merge-commit, which can all be tweaked

@mquinnfd mquinnfd marked this pull request as ready for review August 28, 2024 13:33
- { name: "3.10", python: "3.10", os: ubuntu-latest, tox: py310 }
- { name: "3.9", python: "3.9", os: ubuntu-latest, tox: py39 }
# Static analysis and utilities
- { name: "Verification Tests: Ruff", python: "3.11", os: ubuntu-latest, tox: "ruff" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can drop the prefixes for the names. "Ruff/Mypy/Python 3.X" are specific enough, and I dont really know what a "verification test" is :P

Copy link
Collaborator

Choose a reason for hiding this comment

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

And if you really want to have prefixes, maybe "static analysis" is a better prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need prefixes nah -

I actually had them as Static Analysis but then I added the docs building in so it kinda ruined the fun

I can make it something shorter 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 7d7bfc6

FROM python:3.11-slim AS base

FROM base AS builder
RUN apt-get update && apt-get install -y git
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for installing git here? Where is it used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea! This is cruft from the historical docker image, I assumed the building of binaries required it

Copy link
Collaborator

@cyberw cyberw Aug 29, 2024

Choose a reason for hiding this comment

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

It was needed so that we can determine the locust version name (it was used by setuptools_scm), e.g. for use with locust --version. Does that still work with the new way of building the docker image?

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 believe the Dynamic Versioning plugin for Poetry comes with whatever it needs 🤔
--- but I'll check on the docker image, that a produced version is correct

On the CI, I did make a cut-down docker image which doesn't do any building at all, just takes the already-known-good built assets - the version of this is already resolved in the package (and can be seen in the test-docker-image step)
image

On the "normal" image, which users can still build end-to-end, I can check that the output of locust --version is correct inside the docker image 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out the git binary is a necessity for resolving the version (understandably) even using dunamai etc. so I guess it gets left in, wanted to have the images as close to each other as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does work with the local build as well, seems to get the correct version
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 7d7bfc6

(had to allow dist in the docker context for the CI, so the local one has to remove it (or declare a wheel file name specifically)

docs/developing-locust.rst Outdated Show resolved Hide resolved
@mquinnfd
Copy link
Contributor Author

FYI @cyberw this brings the Docker image build size back down to the same ~90mb it was previously 🥳

@cyberw
Copy link
Collaborator

cyberw commented Aug 30, 2024

I tried to improve the error message for the --json test that was failing 4e29bf6

@cyberw cyberw merged commit 460908d into locustio:master Aug 30, 2024
15 checks passed
@cyberw
Copy link
Collaborator

cyberw commented Aug 30, 2024

the error went away on retry so 🎊

@cyberw
Copy link
Collaborator

cyberw commented Aug 30, 2024

nooo... I didnt squash :-/

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.

3 participants