Unify CI for pydicom projects

[mrbean-bremen]

This is a spin-off of a pynetdicom PR, discussing CI in the pydicom projects. Here is the discussion so far:

mrbean-bremen Jun 19, 2023 •
On a related note, I think it would help to use the same approach for tests/checks for all pydicom core repositories (by that I mean the ones closely related to pydicom, e.g. pydicom itself, pynetdicom, pylibjpeg and the various pylibjpeg plugins, and pyjpegls). I would like to hear what @darcymason thinks on this account - he is currently working on restructuring pydicom for the eventual 3.0 release, and this is something that may also be considered. The tests/checks are currently a bit of a patchwork, created by PRs by different people, and I would like to see a more unified approach for the mentioned repos. Maybe we can create a separate issue in pydicom for this...

sjswerdloff Jun 19, 2023
Ah... One could have a separate group that is for the automated CI/CD, if that level of efficiency is important. More or less same as tests, but cut out the stuff that is for local / development work.
@SimonBiggs has done fantastic work on the automation of pymedphys, and that automation makes maintenance (and being a maintainer) much easier.

darcymason Jun 19, 2023

I would like to hear what @darcymason thinks on this account - he is currently working on restructuring pydicom for the eventual 3.0 release, and this is something that may also be considered. The tests/checks are currently a bit of a patchwork, created by PRs by different people, and I would like to see a more unified approach for the mentioned repos. Maybe we can create a separate issue in pydicom for this...

Agree about the patchwork, and that is why I have been spending the time migrating to pyproject.toml-based config (clearly the future now) and trying to do so in a build-backend-independent way as much as possible. As mentioned in another issue somewhere, I've tried poetry several times over the years and never really got it. And I get the impression that a lot of its functionality has now been moved into pyproject.toml ways that are compatible with other tools. All the build tools are competing now and this space will be a bit messy for a while.

As to running locally, there is act which actually picks up the github actions workflows. I looked at it (or something similar) in the past but I think it didn't have Windows support previously. In any case, it appeals to me because it would be nice to just directly run the github actions. I intend to give it a try soon.

And to continue to be a bit contrarian 😉 I prefer not pinning versions of tools, I'd rather see the error come up and try to deal with it. I recently got over the pydicom mypy pinning - while refactoring anyway, I couldn't remember exactly why it had been pinned, and thought it better to fix up a bunch of problems with the current version, rather than some previous version and then later try to upgrade versions.

In any case, I think act (or similar tool) is likely a permanent solution for CI vs local, if it works.

mrbean-bremen mrbean-bremen Jun 19, 2023

and that is why I have been spending the time migrating to pyproject.toml-based config (clearly the future now) and trying to do so in a build-backend-independent way as much as possible

Yes, using pyproject.toml is now the way to go, though that leaves the question of which tools to use open. I had a try at poetry several years ago and IIRC it didn't impress me, but I guess this has improved together with the whole packaging metaverse. I'm fairly agnostic concerning the used tools, I just want to have some consistence for ease of maintenance. This includes the "meta tools" like poetry, and the actual linters like flake8 etc - would be good to have a single approach. Also, we currently have both command line tools, and GH integrations via annotations, and I tend to get confused with all the different linters.

I recently got over the pydicom mypy pinning - while refactoring anyway, I couldn't remember exactly why it had been pinned

The reason was that the build usually failed in some random PR after a mypy update, and every time this happened it created some back-and-forth with the PR creator as to how to handle these errors, which are completely unrelated to the PR. Also, this used to happen in several PRs at once, so we had to decide, in which one it had to be fixed.
As I wrote, I would like to have both: a pinned version for the PRs, so that the creator of the PR can concentrate on their own changes, and an unpinned version which shows the errors, but does not fail the build. As I understood, pymedphys does something like this, though I may have got that wrong.

In any case, I think act (or similar tool) is likely a permanent solution for CI vs local, if it works.

Independently of what I wrote before, this sounds like a good thing. It also seems to work under Windows now (using Docker Desktop). Pre-commit is also an option - I use it in another repo, though that runs only for the current OS (in an isolated environment).

darcymason darcymason Jun 19, 2023

This includes the "meta tools" like petry, and the actual linters like flake8 etc - would be good to have a single approach.

Of note on the latter, I was really impressed with ruff now that switching to it was contributed in pydicom. Super-fast, and covers everything, including (as in my last PR) upgrading for Python version changes. Mind you, it still uses all the flags from the previous linters, so some of the issues might still follow.

The reason was that the build usually failed in some random PR after a mypy update, and every time this happened it created some back-and-forth with the PR creator as to how to handle these errors, which are completely unrelated to the PR. Also, this used to happen in several PRs at once, so we had to decide, in which one it had to be fixed.
As I wrote, I would like to have both: a pinned version for the PRs, so that the creator of the PR can concentrate on their own changes, and an unpinned version which shows the errors, but does not fail the build

That is a good point. Hopefully quite rare, but I do remember that happening. I'm on the fence as to whether it is worth the extra complication of having different versions in different workflows, vs. the occasional problem like that coming up. Related point, I found it confusing doing the pydicom workflow changes recently, that the same thing is repeated over in several different jobs, in different workflow files. I'd like to minimize that kind of duplication.

CC @darcymason @sjswerdloff @SimonBiggs

[mrbean-bremen]

Hopefully quite rare, but I do remember that happening

I just had a search for these kinds of mypy errors, and it turned indeed out to be only a few items, and the one complaining was mainly myself 😁 :

---

mrbean-bremen commented Dec 19, 2021

Hm, only just noticed that the typing check failed in the last commit - I didn't pay attention as the code had not changed, but mypy has been updated to 0.920 in that run😒

---

mrbean-bremen commented Dec 29, 2021

Yeah, I did all these changes due to mypy 0.930 in my PR, too.

---

kalebdfischer commented Mar 15, 2022 •

Linting failures are not attributable to any change in this PR.

mrbean-bremen commented Mar 15, 2022 •

Linting failures are not attributable to any change in this PR.

That's true - they are probably due to a new mypy version that fixed another issue. This comes up with every new mypy version, and we just fix it it whateve PR it occurs first.

---

mrbean-bremen commented Apr 30, 2022

Hm, now the mypy check failed again due to a new mypy version... Should be sufficient to fix this with the next PR.

---

kalebdfischer commented Nov 8, 2022

Not sure why mypy is complaining about files that are not changed by this MR - I haven't been able to reproduce locally.

---

mrbean-bremen commented Nov 14, 2022

Those typing errors each time a new mypy version is released get a bit annoying. @darcymason - maybe we pin the mypy version (like we did for the patch release) and update mypy ourselves when needed? Or switch to pre-commit, which will create PRs for updated tools, which can then be adapted to pass.
My suggestion would actually be to pin mypy to 0.971 in this PR and handle the new mypy version separately whatever way we decide to.

darcymason commented Nov 14, 2022

My suggestion would actually be to pin mypy to 0.971 in this PR

Sounds good to me.

---

... which we did afterwards.

[darcymason]

Thanks, @mrbean-bremen for putting this into its own issue, and copying over the discussions.

I'm traveling, so only a couple quick comments. I did have a look at one pymedphys workflow and will look at it in more detail. I did like the sparser matrix where not all things are checked or run on all platforms/versions. I'll look into pre-commit a bit more too.

I agree about the different linters, and will see if more can be consolidated into ruff. And I'll try to take another look at circle and see if there are workable alternatives.

As mentioned, I've done some overhaul anyway in pydicom, so my plan would be to keep going with that and see where it leads, keeping in mind the pymedphys example and these other CI tools.

[mrbean-bremen]

Sounds great, thanks!

[SimonBiggs]

And to continue to be a bit contrarian wink I prefer not pinning versions of tools, I'd rather see the error come up and try to deal with it.

-- @darcymason

On this point, the way we handle it in PyMedPhys is we have the PR blocker CI tests run with pinned dependencies, but then we have a separate "updates" test that unpins everything and gives early warning about the issues coming around the corner.

This then frees a new contributor to get their small PR merged, even if the updates would have got the PR blocked. And then a maintainer can do a follow-up PR fixing the update failure CI tests.

[SimonBiggs]

Issue is that for a new contributor, having pre-commit break their PR via an update might be quite painful...

Although I might be wrong with how you intend to use pre-commit...

[mrbean-bremen]

The idea is to be able to run pre-commit locally, and if you want, you can configure it as a commit hook in git (this is what I did). This ensures that the used linter versions are the same as the checked-in ones, and the same checks are run as in the CI. `pre-commit is configured by a config file (example), which is also used by the GH integration.

The update bot creates PRs to update linter versions in this config file, and if the PR build fails, you can adapt the code accordingly (or decide to skip the update), but that is the work of the maintainers. It should not interfere with PRs from new contributors.

I'm of course open to other options, I just wanted to mention that this is something that I have used before (it is used by many repos in the pytest-dev organization), and which works fairly well.

[darcymason]

...we have a separate "updates" test that unpins everything and gives early warning about the issues coming around the corner.

I like this model. I'll definitely try to steal that from your workflows.

The idea is to be able to run pre-commit locally, and if you want, you can configure it as a commit hook in git (this is what I did).

I like this too. I definitely like the idea of pre-commit being used locally, although I don't know if most casual contributors would install it. Still, I'd add info to the contributor docs and pr templates and it would help prevent some problems. I'll look at the pytest-dev workflows too.

I'm going to play with all these ideas in a private repo, so I'll be quiet here for a while...

[SimonBiggs]

I'm going to play with all these ideas in a private repo, so I'll be quiet here for a while...

@darcymason, once you have something you're happy with, PyMedPhys might look to swap to your approach. It might be nice to build a "default" medical physics contributor experience, making it easier for newcomers to feel comfortable providing a small tweak here or there across the ecosystem.

[SimonBiggs]

I'm of course open to other options, I just wanted to mention that this is something that I have used before (it is used by many repos in the pytest-dev organization), and which works fairly well.

@mrbean-bremen, usage of pre-commit.ci appears to be a great idea :) Of note though, the pytest-dev organisation don't use it to enforce automatic dependency updates:

https://results.pre-commit.ci/run/github/37489525/1687338928.A3H5d63QTnS0OvWTjUHJfg

In my opinion, forcing dependency updates within pre-commit is a bad idea. A huge benefit of a lock file, is that a new contributor has a "previously shown to work state" of dependencies within their environment. Forcing them to update their dependencies on every commit effectively undoes this benefit... and we're back to "why is this broken, I didn't change anything to do with that..."

[darcymason]

@darcymason, once you have something you're happy with, PyMedPhys might look to swap to your approach.

Sure, if you like the results. Right now I've been starting with PyMedPhys's workflow, so I'm learning from your team.

[mrbean-bremen]

In my opinion, forcing dependency updates within pre-commit is a bad idea.

You mean, if the user does not want to use pre-commit locally? True, if they want to use their own tools, they have to manage the tool versions manually to match the version in the config file.
If that is what you mean - this is not so different from the case that the user does not want to use poetry, and has to manually synchronize the versions to the versions in the lock file, in my opinion.
Or maybe I didn't understand you correctly...
As I wrote, the dependency updates are separate PRs, which will change the pre-commit configuration only if they are merged by the maintainers.

[SimonBiggs]

Or maybe I didn't understand you correctly...

I guess, I want to be able to install pre-commit locally (for example we ask all contributors to do this), but have it not ever change the lock file configuration.

Instead, have that be an entirely separate process, that is not in any way automatically attached to a commit of someone who is unaware of the impact it might have.

---

On the other hand, running a pyup bot over the repo, independent and unrelated to pre-commit, perfect :)

[mrbean-bremen]

I guess I still don't understand it. Pre-commit is indeed independent of the lock file, as it uses its own environment on execution, entirely based on the config file. You could say that it exists in a parallel universe to poetry, with its own lock file (the config file), and its own update bot (working similar as the pyup bot).

It is true that this could be a problem if the user wants to use their own linters instead of pre-commit, with the versions pinned in the lock file.

[SimonBiggs]

Here's an interesting option...

https://github.com/floatingpurr/sync_with_poetry

[mrbean-bremen]

Ah cool - this would solve the problem of co-existing poetry and pre-commit! Looks like some workable solution is crystallizing... Though let's wait for @darcymason to try things out.

[sjswerdloff]

That seems to work.
I just added pre-commit hooks to a new repo I'm working on.
Also, I found sort and black were fighting it out until I added:

args: ["--profile", "black"]

to the isort hook
(I'm using isort 5.12.0 and black 23.3.0, with pre-commit-hooks at 3.3.0 and sync_with_poetry at 1.1.0)

[darcymason]

I've been looking into all these things and playing with some in the darcymason/egsnrc repo.

My thought process / principles for these choices are:

• keep PR CI time as short as reasonable (prefer <5 min wall time). Sometimes with multiple small fixes, the fast feedback is very helpful
• as discussed, make it easy to have a local venv which matches the online CI tools
• keep the workflow files easy to maintain, not too complex

Here are my thoughts on a reasonable set of tools for pydicom (to be validated with real-world experience in pydicom, of course):

• Use pre-commit.ci as discussed, but not version-related, just to auto-fix or flag issues from black and ruff into any PR, so style/linting issues are handled for the contributor (and us), rather than doing it manually. The pre-commit config file means contributors can also be encouraged to run pre-commit locally.
• in pyproject.toml, pin versions of at least the "problematic" dev tools - mypy and ruff. We probably don't need to pin pytest/pytest-cov, I don't recall ever having problems with them, but OTOH if they aren't causing problems, then that is super-quick anyway, just merge the auto-generated PR from dependabot (next point).
• Use dependabot with weekly updates. This just updates the pyproject.toml with the new pinned dev tool versions, and creates a PR, so the tests automatically run. Thus we can fix any issues coming up, almost always before (unless a new release was out a few days before) they come up in PRs. Contributors can avoid mismatch version problems by pulling latest main and updating local venv tools.
• the workflows will be updated to properly install pydicom, rather than editable install as now. This is part of the advantage of src layout, it forces a 'clean' install to test in. Also will install with [dev] to avoid repeating versions in the workflow files themselves - pinned versions all in pyproject.toml.
• use re-usable workflows for the core and core-extras testing. Eliminates confusing yaml duplication between merge and PR workflows. Alternately (which pymedphys does) we could set flags for the matrix and keep both in one workflow file, but after trying re-usable out, I find it a little cleaner and easier to understand, but either way could work well.
• orthogonal to all the above, I propose to reduce the PR test matrix - just do one Python version per platform (as pymedphys does). Right now in 3.0.0.dev we only have Python 3.10 and 3.11, so I would propose leaving macos platform to the merge PR only (pydicom is very unlikely to have platform-specific errors, and anyway they are still caught).
• Further to last point, slim the merge PR (push to main) down to not include the matrix combinations already tested in the PR.
• I'd like to eventually add doc build to the workflows too, but we can push that down the road a little
• Similarly, I haven't commented here on the other optional dependencies (numpy, gdcm, pillow, pylibjpeg's), but those should get optional-dependency sections in pyproject.toml as well, likely with pinned versions. Again, I'd suggest to get the basic CI working first and then look into those afterwards.

So those are the thoughts - any comments welcome. I think these ideas should play well with the other pydicom org libraries too, but I will starting pushing up PRs to pydicom/pydicom in the next couple of days where they can also be seen in action and we can adjust with experience.

[SimonBiggs]

• use re-usable workflows for the core and core-extras testing.

👍, definitely that over PyMedPhys approach. I don't remember those existing when I built out the CI, and it has been a royal pain not having the CI be able to be split across multiple files and reused.

• Use dependabot with weekly updates. ... Thus we can fix any issues coming up, almost always before

❤️, I love this.

---

Everything in the above appears to me to be at least a pareto improvement above what PyMedPhys is currently doing. If that is the way pydicom went, I would be happy to also have PyMedPhys align and use the same approach. Thanks for digging into all of this @darcymason! 🙂

[mrbean-bremen]

I second @SimonBiggs - this all looks very nice!

Use pre-commit.ci as discussed, but not version-related

This is ok - we can still switch to the bot to create PRs for tool version updates later if wanted.

in pyproject.toml, pin versions of at least the "problematic" dev tools - mypy and ruff

Agreed. I'm not sure though how these interact with the version of the same tools in the precommit config, but maybe you have already checked this.

Use dependabot with weekly updates.

Ok - if mypy and ruff are also there, this would update them too, so no need for the pre-commit update bot, which does the same. Same here - not sure how this works together with the pre-commit config.

use re-usable workflows for the core and core-extras testing

👍🏼

Also, very much agree to reduce the number of (redundant) tests to be able to have fast feedback.

I'd like to eventually add doc build to the workflows too, but we can push that down the road a little

Yes. We just have to make sure to have a decent preview for documentation changes, though this is solvable. There is an example how to do this in GH Actions that I have used recently (essentially by checking in the changes into another directory in the ghactions branch, and removing them after the PR has been merged). There is also the possibility to switch to Read The Docs instead, which does this out of the box (I have done this also, and it is an easy step, too).
But I agree, this can wait.

I will starting pushing up PRs to pydicom/pydicom in the next couple of days

Yes, please go ahead!

[darcymason]

I'm not sure though how these interact with the version of the same tools in the precommit config, but maybe you have already checked this.

I've been looking into it. I will start with duplication and then get a better solution. I thought to modify sync-with-poetry (probably create our own from it) but have now also discovered pyproject-pre-commit which is itself a pre-commit hook that then calls the linters etc. It doesn't have ruff at the moment, but perhaps we could contribute it, or even just make our own pre-commit hook based on this.

Yes. We just have to make sure to have a decent preview for documentation changes, though this is solvable. There is an example how to do this in GH Actions that I have used recently (essentially by checking in the changes into another directory in the ghactions branch, and removing them after the PR has been merged).

Okay, I'll try to remember this so we can circle back to it.

There is also the possibility to switch to Read The Docs instead, which does this out of the box (I have done this also, and it is an easy step, too). But I agree, this can wait.

We switched from readthedocs years ago, I'd be reluctant to go back ... unless somehow can just point github pages to it?

[mrbean-bremen]

but have now also discovered pyproject-pre-commit which is itself a pre-commit hook

Yes, that looks promising. I also had a look and found this issue and a few linked ones that show that some integration is wanted by more people, and why it will not be done.
I also thought about directing the pyup bot to the pre-commit config file, but it only understands the format used in requirements.txt.

We switched from readthedocs years ago, I'd be reluctant to go back ... unless somehow can just point github pages to it?

Well, you can just point ReadTheDocs to the same documention config that we use in GH Pages, no changes in the repo are needed except for a new config file - but actually it wouldn't be much different to the existing CircleCI configuration.
We can discuss this later, I have no strong opinion either way.

The other possibility I mentioned uses a custom GH Actions workflow. I used this in another project, and while it is a bit clunky, it works fine. It creates a comment with a link to the preview in a PR, and removes it after the merge.

[darcymason]

I'm not sure though how these interact with the version of the same tools in the precommit config, but maybe you have already checked this.

I've been looking into it. I will start with duplication and then get a better solution.

So, on my egsnrc repo where I was testing various things, I now have a pre-commit-ci auto-PR which updated ruff's version in the pre-commit-config.yaml file. So it seems there is a setting somewhere in the pre-commit github app. I'll look for it when I attach it to pydicom. And we should try to more or less synch the weekly check - perhaps just committing changes in both files together may work.

On another note, ruff has updated twice already in a few days (I had my test repo on "daily" check), so we may be getting those almost every week. But with small frequent updates the chance of any differences in someone's PR are likely also very small.

[mrbean-bremen]

I now have a pre-commit-ci auto-PR which updated ruff's version in the pre-commit-config.yaml file.

Yes, this is what I am using for pre-commit, too. As far as I remember I just use the default, and I get update PRs about once in 2 weeks (that is mainly with black, flake8 and mypy - didn't include ruff). I also use it together with the pyup bot for the requirements files, these work very similar.

But with small frequent updates the chance of any differences in someone's PR are likely also very small.

Agreed. Usually it is just a clean merge, and if not, there is no rush to merge - the merge PRs update themselves with other merges, so there will be no conflicts if they stay open.

[mrbean-bremen]

In case you wonder that I always write pyup bot instead of dependabot - I was actually using that, but as far as I can see, the functionality is mostly the same. Auto-rebasing on other merged PRs can be also be configured in dependabot, as it seems to currently be off.

[darcymason]

In continuing news, I have done/am working on:

• changing from codespell to typos
• removing LGTM (which is now deprecated and transitioned to CodeQL, hosted by github)
• I have turned on CodeQL in github Setting | Security and Code Analysis

[darcymason]

@SimonBiggs, I think my workflow changes are fairly firmed up now, in #1834, if you want to have a look.