migrate to Poetry

[danieleades]

No description provided.

[danieleades]

@danwos I wondered if I could get a few pointers about the test environment before i go down any rabbit holes...

i've done the minimal work to restructure the project to use Poetry, and i'm attempting to run the testing to make sure this is working.
I haven't touched tox yet, but have just been trying to run pytest

poetry install
poetry run pytest

which complains about a missing test fixture on each test

E       fixture 'app' not found
>       available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, doctest_namespace, monkeypatch, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory

Is there something about the way you're running tests that sets up the environment to include the app fixture (tox or nose maybe)?

[danwos]

732 file changes looks a little bit too much :)
We also need to integrate the change into tox and travis config files.

[danieleades]

732 file changes looks a bit too much :)

Haha terrifying isn't it? Looks like the restructure has double-counted the changes- once for the deletion and once for the addition of each file. I might be able to do something about that.

We also need to integrate the change into tox and jenkins config files.

Of course. I wouldn't mind getting the tests passing locally as a first pass though. I don't have any concerns about getting Poetry, Tox, and Jenkins to play nicely together after that

[danwos]

Haha terrifying isn't it? Looks like the restructure has double-counted the changes- once for the deletion and once for the addition of each file. I might be able to do something about that.

I often have this problem when file attributes got changed (mostly by some unknown voodoo on os level).
Maybe this helps to not report this kind of changes: git config core.fileMode false
See also this SO question.

[danieleades]

that's the file renaming issue sorted. there's still 655 file "changes" but the majority of these are simply due to a small restructuring of the directory and are not actual changes.

[danieleades]

@danwos I wondered if I could get a few pointers about the test environment before i go down any rabbit holes...

i've done the minimal work to restructure the project to use Poetry, and i'm attempting to run the testing to make sure this is working.
I haven't touched tox yet, but have just been trying to run pytest

poetry install
poetry run pytest

which complains about a missing test fixture on each test

E       fixture 'app' not found
>       available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, doctest_namespace, monkeypatch, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory

Is there something about the way you're running tests that sets up the environment to include the app fixture (tox or nose maybe)?

still seeing this issue. I strongly suspect i've got a version mismatch between sphinx and sphinx-testing that's causing issues.

what versions of sphinx do you wish to support? I see that 'sphinx-testing' is deprecated on the current version of sphinx

update: downgrading the sphinx version 3.2.1 -> 1.8.5 gives me the same error

[danwos]

sphinx-testing is old, I know. And it is using nosetest! Not pytest.
pytest is only used for flake8 tests.

To be honest, I would like to use the official way of sphinx running its tests.
But this would need a lot of time, as some test-cases needs to be rewritten and maybe use different mechanisms.

Maybe we should start using the "new/current" way of writing tests for sphinx (pytest), without touching existing tests.
Then the project has some good example how to do it and later transformation should be much easier or can be done step-by-step.

The tox. file is running the tests with versions between >=2.2 and < 3.1.
So 1.8 is official not supported (has worked in the past, but some functions may now use the newer sphinx api.)

[danieleades]

aha! ok, getting warmer.
I don't have a lot of experience with nosetest but I'll start exploring. Sounds like porting to pytest is a problem for another day...

[danieleades]

that's the tests running! (make test)

there's two failures, that'll require a little more investigation.
next i'll try to integrate tox.

[danieleades]

that's a minimal working tox configuration added back in.
I've stripped a lot out of it to get this working, and it's currently only using different interpreters in the matrix (not different sphinx versions)

If I try to install multiple sphinx versions I end up with multiple inclusions of sphinxcontrib.serializinghtml which causes a circular import. I think this is because tox is installing it using pip, and not deconflicting it with the other packages installed by poetry. This will take a little more thought.

a couple of questions

• which interpreter versions and which versions of sphinx do you want to support? Shall I endeavour to match the existing tox configuration, or did you want to take the opportunity to update this?
• is it necessary to do the linting using tox? it seems to me that you would want to do the formatting with a pinned version of whatever library, and do linting with the latest available. Does this need to live inside the tox matrix, or can this just be a simple makefile target?

[danieleades]

it looks like Tox isn't going to work with Poetry. Turns out there's a lot of github and stackoverflow issues with people trying to figure this out.
I've been playing around with a newer alternative called Nox. It's conceptually similar, but uses python files to run the builds instead of an opaque configuration file. I think the extra flexibility afforded by this will break the deadlock.

see this discussion here - python-poetry/poetry#2920, and here - wntrblm/nox#347

this is turning out to be a much larger set of changes than I originally anticipated, though of course there are 0 actual code changes. How are we feeling about this @danwos? Have I gone a little off-script?

[ubmarco]

I'm using tox with Poetry in another project:
https://github.com/useblocks/libpdf/blob/master/tox.ini
The project also uses Github actions as a CI solution:
https://github.com/useblocks/libpdf/blob/master/.github/workflows/tox.yml

[danwos]

@danieleades thanks for all your effort. 👍

Linting should be part of tox and it can use its own environment (like flake8 does).
Reason for this: The developer can run simply tox and all tests and checks are fired.
And it behaves the same way as on CI, so if it passes locally it will pass also our CI.

An update of supported sphinx/python versions is normally done if:
a) A new, important Python or Sphinx version was released
b) If a feature got implemented, which needs to use an API, which is available on newer version only. Then old versions get thrown out.
So current version of Sphinx is 3.2 and tox is testing <=3.0 only. So 3.2 may be added. The rest is fine for me.

About poetry: I still think that there are 0 code changes needed. But for sure some configuration changes (tox, mabye travis, poetry itself). It's still okay for me to change.

[danieleades]

@ubmarco

I'm using tox with Poetry in another project:
https://github.com/useblocks/libpdf/blob/master/tox.ini
The project also uses Github actions as a CI solution:
https://github.com/useblocks/libpdf/blob/master/.github/workflows/tox.yml

to be clear, the issue is not Poetry and Tox (this pull request is currently using poetry and tox together), the issue is running a tox build with poetry and a matrix of dependencies. I see that in your project you do not have any such dependency matrix.

the issue is well captured in this thread, and there is a workaround proposed in this comment, however this doesn't work in this case because the separate dependency resolution (pip and poetry) results in multiple copies of sphinxcontrib.serializinghtml which causes a circular import (see this issue)

[danieleades]

that's the project updated to use nox instead of tox. Turns out to be much tidier with Poetry

[danieleades]

@danwos i've reworked this to stick to the old directory structure. You'll notice the 'files changed' is now a far less scary '10'

I've added a short 'contributing' document with the required dependencies and a quick description of the endpoints. I chucked in a makefile for convenience.

One strange thing i'm noticing- after running the tests (poetry run nosetests -w tests) some of the images in /docs/_images/ are reported by git as changed. Only affects a subset of the pie charts. Have you seen anything like this before with these tests?

[danieleades]

I think this is ready for review.

open questions-

• weird git behaviour around the images after running the tests
• what version constraints do we want for each of the dependencies (I simply allowed Poetry to grab the latest, but this might not be what you want!)
• Are we happy with Nox? I think it's a better solution than Tox, but I also know that Tox is pretty ubiquitous (see migrate to Poetry #154 (comment))
• I haven't touched the travis config yet. This is a little tricky for me to debug. It looks like travis is not configured to run tests and comment on pull requests.

[ubmarco]

Hi @danieleades,
thanks for the feedback in #154 (comment).
I see the problem. Also thanks a lot for your contribution. Here are some notes for the PR:

I tried to run the tests in your branch but get an error on a fresh checkout:

$ poetry run nox --session test
Creating virtualenv sphinxcontrib-needs in /home/marco/ub/danielleades/.venv

[FileNotFoundError]
[Errno 2] No such file or directory

At least nosetests and nox need to be preinstalled to run make test or make test_matrix. It works after installing all dependencies first with poetry install or have them preinstalled locally to the user's home directory. Maybe add that to the contribution guide? There is also an open issue for Poetry for having a new option that only installs dev dependencies.

After installation of deps I get:

poetry run nox --session test
nox > Error while collecting sessions.
nox > Sessions not found: test

I guess there is a typo in the Makefile for test_matrix (test vs tests).

I see that the Sphinx dependency is given as ^3.0. That can be simplified to ^3 because the caret specification honors SemVer.

[danieleades]

Hi @danieleades,
thanks for the feedback in #154 (comment).
I see the problem. Also thanks a lot for your contribution. Here are some notes for the PR:

I tried to run the tests in your branch but get an error on a fresh checkout:

$ poetry run nox --session test
Creating virtualenv sphinxcontrib-needs in /home/marco/ub/danielleades/.venv

[FileNotFoundError]
[Errno 2] No such file or directory

At least nosetests and nox need to be preinstalled to run make test or make test_matrix. It works after installing all dependencies first with poetry install or have them preinstalled locally to the user's home directory. Maybe add that to the contribution guide? There is also an open issue for Poetry for having a new option that only installs dev dependencies.

I've added a line to the contribution guide that explains that the dependencies must be installed before running the other 'tasks'.
I don't think we want an option that only installs dev dependencies here because the non-dev dependencies are also required for testing.

After installation of deps I get:

poetry run nox --session test
nox > Error while collecting sessions.
nox > Sessions not found: test

I guess there is a typo in the Makefile for test_matrix (test vs tests).
quite right! there was indeed a typo in the noxfile.py

I see that the Sphinx dependency is given as ^3.0. That can be simplified to ^3 because the caret specification honors SemVer.

You're right, these are resolved in the exact same way. I prefer to use the 3.0 form for two reasons

1. this is the default format used by Poetry when adding dependencies (poetry add ${dependency})
2. for consistency with the other dependencies.

It is an open question (listed above) how strict this pull request should be with dependency requirements. There are basically two approaches

• specify the latest versions on the assumption that users are using modern systems or isolated environments (like Poetry) and won't have any issues getting the latest releases
• use version ranges instead of caret requirements, and test across the range of dependencies

This repo is clearly leaning towards the second option, which is the most flexible. I guess I need some input from the maintainers on the extent to which this should be done, and for which dependencies.

[danwos]

I would like to keep the matrix-test coverage which we already have. Means:
Python: 35,37,38
Sphinx 22, 23, 24, 30, 32 (new)

I see now reason why we should pinch out some of them.

Also the poetry deps seems to be too restrictive for me (but I'm not a poetry expert).
Sphinx is set to sphinx = "^3.2.1", this means sphinx-needs can't be installed with Sphinx 2.x.
From the code I see no reason why this shouldn't be possible.

My philosophy for a lib is: Force as less as possible for specific dependencies version.
So as long as we have no technical reason to force the usage of a specific version, I wouldn't set any version information.
E.g sphinx-copybutton.
Main reason: A lib is used together with other project constrains (app, libs ,...), which are under the responsibility by the user.
If all libs would have such strict deps, an installation gets nearly impossible.
This counts especially for lib-dependencies, which are used by other projects parts with own requirements as well (tests = pytest, nose // app = pathlib, nox).

I had a similar discussion with the maintainer of another project, where the version were pinned very strictly and therefore I couldn't use the lib.

Please tell me if you have other thoughts about it. I'm open to learn :)

[danieleades]

@danwos that all makes sense. Ive got caret requirements in there only because it's the default. Poetry supports version ranges. I will update to include the ranges you mention.
With respect to the dev dependencies however, the fact that the project uses an isolated environment (through poetry) means that there's no reason not constrain the requirements much more tightly.
For testing and linting dependencies you probably want the latest matching major version. For formatting dependencies you probably want to go a step further and pin to a specific version

I can imagine one case where you might run into some challenges here is if your test dependencies shared a lot of transient dependencies with your non-dev dependencies. I could imagine situations where this breaks the dependency resolution. Note that this would only affect you during testing, and not at runtime.
You could even just use 'wildcard' dependencies here if you really need to...

[danieleades]

that's the dependencies and test matrix updated to cover the same range as the original

[danieleades]

This will need more work specifying the dependency versions as a function of python version. I won't get a chance to work on this further until next week, unfortunately

[danwos]

No problem, I'm also not availabe this week. But my goal is to support/check the PR finally next week and maybe implement some missing/open decisions by myself.

[danieleades]

got a bit of time now.
Here's the current state of the test matrix-

nox > Ran multiple sessions:
nox > * test-3.5(sphinx='~2.2'): success
nox > * test-3.5(sphinx='~2.3'): success
nox > * test-3.5(sphinx='~2.4'): success
nox > * test-3.5(sphinx='~3.0'): failed
nox > * test-3.5(sphinx='~3.2'): failed
nox > * test-3.6(sphinx='~2.2'): success
nox > * test-3.6(sphinx='~2.3'): success
nox > * test-3.6(sphinx='~2.4'): success
nox > * test-3.6(sphinx='~3.0'): failed
nox > * test-3.6(sphinx='~3.2'): failed
nox > * test-3.7(sphinx='~2.2'): success
nox > * test-3.7(sphinx='~2.3'): success
nox > * test-3.7(sphinx='~2.4'): success
nox > * test-3.7(sphinx='~3.0'): success
nox > * test-3.7(sphinx='~3.2'): success
nox > * test-3.8(sphinx='~2.2'): success
nox > * test-3.8(sphinx='~2.3'): success
nox > * test-3.8(sphinx='~2.4'): success
nox > * test-3.8(sphinx='~3.0'): success
nox > * test-3.8(sphinx='~3.2'): success

looks like sphinx >=3.0 doesn't like python <3.6
that might be resolvable though, i've not done any playing around with this yet.

I think the next steps are

1. play around with the requirements bounds until we're happy with the matrix
2. start incrementally relaxing the bounds for each requirement until we find the least restrictive working state

edit: sphinx >=3.0 claims to support python 3.5+. Suspect the reason this is failing is a bit subtle.

edit: in order to get these results i've had to disable one test which is failing on every configuration (and has been since before I started making modifications!).
The matrix as shown here is actually already consistent with the original pre-Poetry repo. In the original setup.py file python 3.6 is outlawed, and it is specified that for python 3.5, we need sphinx <3.0. Given those exclusions, all the test cases are passing. The question is whether we can do any better than that...

[danieleades]

I'm using pyenv in my test environment to manage multiple versions of the python interpreter. Nox supports this, so there's no problems.
Except i've discovered that Poetry is looking for an interpreter that matches the version specification in the pyproject.toml file and using that one. This seems to clobber Nox's attempts to use the specified python version.
This means that no matter what python version I ask for in Nox, i'm getting python 3.8 when i run my tests.

back to the drawing board.

[danieleades]

last issue is resolved, and i'm correctly using each interpreter version in the test environment. no issues there.

next problem-
poetry is unable to solve for the dependencies for python the full range of dependency and interpreter versions stipulated above
In fact the solver just got a big update and now rejects more combinations than it did previously! (i previously had a couple of 'solutions' with 3.6, and a few with 3.5)

There's a couple of possibilities here

• a solution does exist, but poetry can't find it
• a solution never existed and the previous test coverage just never identified the issues
• the semantic versioning of the packages is incomplete, or doesn't reflect their true compatibility (for at least the portion of API which is used in this project directly or transitively)

I suspect it's likely the third case. Especially when you consider that semantic versioning and dependency management in Python projects has been a complete free-for-all until relatively recently. Many packages simply didn't specify working ranges in earlier versions. This means that its very likely there are working configurations which Poetry will not accept.

So what's next?

either

• this pull request can be abandoned
• it can be accepted with the tighter dependency restrictions

there are really only two important variables in the dependency solving- the interpreter version and the sphinx major version
(there are more edge cases then these, but this broadly captures the trend

python	sphinx	works
3.5	~2.2	n
3.5	~3.8	n
3.6	~2.2	y
3.6	~3.8	n
3.7	~2.2	y
3.7	~3.8	y
3.8	~2.2	y
3.8	~3.8	y

python 3.5 - no solutions
python 3.6 - some solutions, with careful dependency specification. A lot of cases where just 1 or 2 unit tests fail (so probably solvable with some effort)
python 3.7+ - everything works

i guess it's up to the maintainers how they want to proceed.

Personally, I think this project is taking on an unnecessary maintenance burden by trying to support python 3.5 + 3.6. This project is a documentation generation tool, so in most realistic applications it's going to be used in a controlled environment like a virtual environment, a docker container, a continuous integration server, or some combination of the above.

[danwos]

Sorry for my really late feedback and thanks for all of your investigations.
As this PR importantly updates our infrastructure and makes it ready for the future, I really want to have it :)

But I have to test in on my own and I have to learn and understand 1-2 new tools (nox and poetry), so this may need some time.
So a finale merge may need some more weeks (sorry, corona chaos here), but it will definitely be part of the project one day.
I will come back, if I have any questions after my testing.

[danieleades]

Sorry for my really late feedback and thanks for all of your investigations.
As this PR importantly updates our infrastructure and makes it ready for the future, I really want to have it :)

But I have to test in on my own and I have to learn and understand 1-2 new tools (nox and poetry), so this may need some time.
So a finale merge may need some more weeks (sorry, corona chaos here), but it will definitely be part of the project one day.
I will come back, if I have any questions after my testing.

no problem, i totally understand. this is a big change, and seems to be having some unexpected effects on the test executions that will require investigation by someone with a more detailed knowledge of the code...

I'll put it on ice for now, and we can revisit.

I'm already using Poetry locally to install the dependencies for this project (I don't like installing system-wide dependencies if I can help it, and Pip doesn't always play nicely with apt-get) and that works really well. It's just Tox/Nox that creates the challenge.

nox-poetry looks promising

[danieleades]

closing in favour of #216

rather than dealing with an ugly rebase, i've started fresh, with a slightly different approach.

turns out most of the dependency errors i was getting here where from incompatibilities with project dependencies and 'nox' dependencies.

i can solve this by stipulating that Nox is a system-wide dependency, and is not not installed in the isolated environment.