[MRG] Update pyproject.toml and tidy-up ci cd #841
Conversation
corrections to allow poetry install -E docs poetry install -E tests to work about as one would expect
… what was being selected updates to pyproject.toml to ensure that all dependencies needed for pytest, coverage, mypy, pylint, flake8, etc are in it. updated poetry.lock
|
There's probably a bit of fluff (unnecessary packages) in the pyproject.toml, it was based on a combination of pydicom and pymedphys. I pulled out most of what I knew wasn't relevant from pymedphys. |
…cated set-output command to GITHUB_OUTPUT added requestor/requestors as allowed spelling (Kings English?)
…ing approach of direct touch of pydicom source
… in the all group
…have a name so i put a name in the project.urls section
Even though I had it set to <6 CircleCI was using v7.0.1 (latest). I'd like to see if CircleCI is ignoring the pyproject.toml with respect to sphinx
CircleCI insists on using a newer version of Sphinx (it starts by referencing 7.0.1 and then uses 6.2.1) maybe having a newer version of pythong will help. Either that or we need to hunt down the error / obsolescence I found earlier having to do with % needing to be escaped
Codecov Report
@@ Coverage Diff @@
## master #841 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 28 28
Lines 8639 8639
=========================================
Hits 8639 8639 |
|
the python 3.9 release test hung on test_fsm, which is a test I've seen hang elsewhere. |
sjswerdloff
changed the title
…sphinx, which works for sphinx 7.0.x
| with: | ||
| output: '\n' | ||
| fileOutput: '\n' | ||
| - name: Get Python file diff | ||
| id: pydiff | ||
| run: > | ||
| echo "::set-output name=pyfiles::$(echo -e | ||
| echo "pyfiles=$(echo -e | ||
| '${{ steps.diff-files.outputs.files_added }}\n${{ steps.diff-files.outputs.files_modified }}' |
There was a problem hiding this comment.
Thanks - time to get rid of these deprecation warnings!
There was a problem hiding this comment.
my pleasure.
deprecation warnings from the build system /GitHub are very high priority for me because those items obsolescing can be extinction events for a project.
The deprecation related to security issues. Sadly, even the FOSS space needs to be more careful about that.
I'm doing some development that leverages pynetdicom so I am inclined to tidy things up a bit. Some of the work is just slight enhancement of existing examples.
Other bits are substantial enough that I am doing it my own public project space.
Perhaps when it matures it can go into apps.
| @@ -0,0 +1,1740 @@ | |||
| # This file is automatically @generated by Poetry 1.5.1 and should not be changed by hand. | |||
There was a problem hiding this comment.
This looks like a lot of dependencies. Just as an example: does sqlalchemy (which I guess is needed by some other package) really need to have drivers for Oracle, Postgres, MySQL and MariaDB? Does sphinx really need all these plugins (like AppleHelp and QtHelp)?
I don't have experience with poetry, so I would ask somebody with more knowledge (@SimonBiggs?) to have a look here.
There was a problem hiding this comment.
The dependencies are those specified by the packages that are explicitly used in pynetdicom.
There was a problem hiding this comment.
SqlAlchemy is explicitly used in apps/qrscp.py
So whatever it needs has to be incorporated.
I don't know if there is a SqlAlchemy subset that could be referenced that would avoid the drivers. pynetdicom is currently only using sqlite, so thats the only "driver" we would need.
There was a problem hiding this comment.
Thanks - it just caught my attention, though I leave that to @SimonBiggs.
| # Tests # | ||
| # ----- # | ||
| pytest = { version = "*", optional = true } # groups = ["all", "tests"] | ||
| pytest-sugar = { version = "*", optional = true } # groups = ["all", "tests"] |
There was a problem hiding this comment.
The CI/CD uses these, so a developer needs them to ensure that they will pass the tests.
They are referenced only for the installation for someone performing tests.
| # astroid = { version = "*", optional = true } # groups = ["all", "tests"] | ||
| # psutil = { version = "*", optional = true } # groups = ["all", "tests"] | ||
| pylint = { version = "*", optional = true } # groups = ["all", "tests"] | ||
| pytest-rerunfailures = { version = "*", optional = true } # groups = ["all", "tests"] |
There was a problem hiding this comment.
Not the ones I commented out, so I should remove those, even though they have no impact because they are commented out.
The ones not commented out are used by CI/CD so the developer/tester needs them to perform those tasks before committing
There was a problem hiding this comment.
Ok, thanks. I was only referring to pytest-rerunfailures (and pytest-sugar before), as these also should not be needed for local tests.
As for the commented out things - yes, you can just remove them (though astroid is needed by pylint, as far as I know...).
There was a problem hiding this comment.
Aa a developer running pytest, having the ability to only rerun failures is important for a tight loop on fixing something. I suppose I could also just specify the particular tests involved but rerun failures pretty much automates it.
There was a problem hiding this comment.
Sure. Generally, I like to have a configuration that allows to run the tests locally the same way as in the CI, and I understand that in this case there will be a few dependencies that are not needed for CI, but very helpful locally. I usually have my local venv that I use for testing that has what I need, but this way it is more consistent.
On a related note (not directly related to this PR), 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...
There was a problem hiding this comment.
But one step at a time.
Yes - I didn't mean this for this PR, it was just a convenient place to put it. This should ultimately handled separately, should we decide to do it.
And yes, it's all about making maintenance and being a maintainer easier, given the limited amount of these 😄
There was a problem hiding this comment.
I would like to hear what @darcymason thinks on this account - he is currently working on restructuring
pydicomfor 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 inpydicomfor 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.
There was a problem hiding this comment.
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 petry, 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).
There was a problem hiding this comment.
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
mypyupdate, 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.
There was a problem hiding this comment.
I copied this to a separate discussion issue unrelated to this PR - I propose to discuss this further over there.
| # ---------------- # | ||
| # Development Only # | ||
| # ---------------- # | ||
| pre-commit = { version = "*", optional = true } # groups = ["dev", "all"] |
There was a problem hiding this comment.
pre-commit would also make sense to setup, but we we don't use it, right?
Maybe I don't understand the purpose of all these optional packages - are they there to facilitate local checks?
There was a problem hiding this comment.
I agree that we should use precommit hooks.
I hadn't realised that I didn't comment that out.
There was a problem hiding this comment.
Maybe I don't understand the purpose of all these optional packages - are they there to facilitate local checks?
Yes, intended for local checking.
A next step would be to have most of the CI/CD checks defined so that they could be run locally using poetry, e.g.
poetry run tests
And instead of explicit build for docs using Make, a similar thing could be done via poetry, or at least the html smoke test/doc generation.
@SimonBiggs knows more.
| @@ -29,3 +61,59 @@ ignore_missing_imports = true | |||
| disallow_untyped_calls = true | |||
| disallow_untyped_defs = true | |||
| disallow_incomplete_defs = true | |||
|
|
|||
| [tool.poetry.dependencies] | |||
| python = ">=3.7,<3.11" | |||
There was a problem hiding this comment.
I intend to comment more about the various discussions around, but having a quick moment, I wonder here about this poetry-dependent part, when there are [project] dependencies and [project.optional-dependencies] which are build-tool agnostic. Does poetry not support the more general options now?
There was a problem hiding this comment.
I don't know.
I basically copied out of pymedphys.
I will take a more careful look at the poetry documentation (it seems a bit skimpy on the content of the pyproject.toml).
I am going to split out the sphinx fix, and also the deprecated github workflow fix.
That will effectively leave this PR for just the pyproject.toml changes
|
There was too much in this pull request, with one part being controversial. |
Reference issue
#839
Touchy CI/CD. This is only a start in the right direction.
Tasks
This allows a contributor to use poetry to install the dependencies and get the right ones:
poetry install -E all