migrate to poetry

[dbarrosop]

@dmfigol @ktbyers

I opened this PR which introduces poetry, however, there are two problems with poetry, one of them which may be a blocker:

1. Minor issue, poetry doesn't seem to be able to read the version from a file or from python code, so we either need to remove version.py or duplicate the version (don't like it but it's not a big deal)
2. poetry is insanely slow. Just run time make build_test_container in the develop branch and then on this one, you will see it's untenable. There is an issue in poetry repo but I don't know the state of it; Poetry package install time seems longer than installing with pip python-poetry/poetry#338

[dbarrosop]

Mmm, surprisingly it's not that slow in travis, maybe it's a weird interaction with docker on mac :S

[dmfigol]

Looks good. Just a couple of comments.
Regarding your questions:

1. I found a workaround here which I tested and it works.
   TL;DR: __version__ = pkg_resources.get_distribution("nornir").version
pkg_resources is shipped with setuptools, but since we are on Python 3.6, the setuptools is shipped with the Python.
2. For all of my projects it is not slow. ("slow" is relative, and it is slower than pip, but not significant) I am not sure how we could troubleshoot it. Do you have some reference time? Notice something strange during the build stage?

[dmfigol]

I think the versioning should be evaluated on per package basis..
For example, I'd propose to keep requests as *. Something less stable as pydantic could be pinned more aggressively. The main reasoning why - when a new (major) version of something like requests comes, nornir users will not be able to update to it, until we change in nornir

[dmfigol]

Just to double-check, are you ok with versions >= 2.0.0 and <= 3.0.0? I thought you would leave something like ^2.18.1 so that it translates to >= 2.18.1 and <= 3.0.0

[dmfigol]

For all of my dev dependencies I do "*" and do poetry update periodically. It really helps to improve the code earlier, especially with mypy and black, which are actively developed and new features come out quite often.

[ktbyers]

Shouldn't black be black==18.6b4 (i.e. this is what we are currently specifying)?

[dbarrosop]

I want to upgrade it as it has improvements to the internal API that some IDE plugins require. Shouldn't change anything on the code (at least it doesn't change anything on nornir or any of my other projects)

[dbarrosop]

Disregard my comment about slowness. It's a problem with docker for mac. It's slower than pip but it's not that bad when using a regular VM.

[ktbyers]

I would have thought freezing development dependencies would be one of the main objectives here?

In other words, a meaningful annoyance is development dependencies causing the build to break due to changes in those libraries (i.e. totally unrelated to any PR, the unit tests break).

So I definitely vote to freeze everything in requirements-dev.txt. I probably care less about the sub-dependencies of the things in requirements-dev.txt (since this seems to cause fewer build breaks than the main dependencies in requirements-dev.txt).

Black I think we should freeze for extended periods of time. Black updates that cause meaningful code formatting changes would be pretty ugly for outstanding PRs and the needed rebasing. Or worded differently, we get 95% of the value from having auto-formatting and the incremental value of new versions of black isn't that great relative.

My vote on the top-level dependencies (i.e. requirements.txt) is generally to have a >= requirement so that we get the newest version by default. NAPALM and Netmiko should probably be regression tested against Nornir prior to releasing them and I wouldn't want Nornir users to be actually using downrev versions of these libraries. And forcing downrev versions would significantly slow down getting fixes to people.

[dmfigol]

@ktbyers
Having * in pyproject.toml does not impact "freezing" dependencies, since dependencies are installed from the lock file, which always freezes them.
Having * in dev dependencies allows you to update dependencies to the latest non-conflicting ones (based on the constraints and resolver) with a single command: poetry update.
So the workflow to update dev dependencies for the project is: poetry update -> your venv is updated + poetry.lock is updated -> PR -> merged -> (other devs) git pull -> poetry install
Non-dev dependencies with the constraints (from pyproject.toml) are included in the metadata of the wheel file which users download from pip. I like your >= idea.

[ktbyers]

Having * in pyproject.toml does not impact "freezing" dependencies, since dependencies 
are installed from the lock file, which always freezes them.

Okay, makes sense.

So the workflow to update dev dependencies for the project is: poetry 
update -> your venv is updated + poetry.lock is updated -> PR -> merged -> (other devs) 
git pull -> poetry install

Okay, makes sense...thanks.

[ktbyers]

I think we should focus on what is the desired high-level workflow and then how do we map that into Poetry (as far as dependency updates, testing, and what gets installed by end-users using the library).

Here is my view on it:

1. On every travis-CI build the latest Netmiko, NAPALM, Paramiko released versions would be installed (i.e. for what used to be in setup.py/requirements.txt we should be testing with latest released version). Excluding probably pydantic and mypy_extensions which I am more inclined to pin to a specific version (at least in the near term). I have no idea how much of a risk the mypy_extensions are though.

2. On every travis-CI build NONE of the development versions are updated i.e. they are frozen. And they are only updated when one of the maintainers explicitly takes the task to update them and to update the lockfile for them.

3. IMO Black should be on an even longer update cycle i.e. should be frozen in the pyproject.toml (per the reasons I mentioned earlier). In other words, this would not typically get updated even with a poetry update.

4. End users should be installing the latest version of Netmiko, NAPALM, Paramiko (i.e. for what used to be specified in setup.py/requirements.txt) excluding Pydantic and maybe mypy_extensions.

What are other people's thoughts on this?

[dbarrosop]

Based on your feedback I am going to start setting the following workflow, we can iterate on it later on as we move forward and we see what works at what doesn't:

1. For the application dependencies:
   a. if semver is supported we pin to major release
   b. if semver is not supported we pin to specific version
2. For development:
   a. black is pinned to a specific version
   b. everything is set to *
3. PRs can't update dependencies, if development or application dependencies need to be updated we will have a dedicated PR
4. Prior to a release we will update dependencies

[dmfigol]

@dbarrosop the problem with pinning non-dev dependencies (even to a major release) is that once a new major version of that dependency is out and it is user app's dependency too, they can't update to it, until the new nornir version is released.
If those users use poetry or pipenv, nornir installation may fail at the resolving phase.
If you are ok to release a minor nornir version every time any major release of a prod dependency is out, then we can accept this.

[dbarrosop]

Libraries don't change major version that often and when they do there is a compelling reason for that so we will want to look into it. For instance:

1. Jinja2 has been in the 2.x release train since 2008
2. requests has been in the 2.x release train since 2013
3. rest have been in their respective major versions for years as well and they had compelling reasons for the change

So basically, I don't see this as a problem and it clearly documents what we support.

[dmfigol]

I like how the whole lifecycle is happening now, including build/deploy.

[dmfigol]

Reading from disk and parsing toml file twice.
This WA __version__ = pkg_resources.get_distribution("nornir").version didn't work?

[dmfigol]

Just to double-check, are you ok with versions >= 2.0.0 and <= 3.0.0? I thought you would leave something like ^2.18.1 so that it translates to >= 2.18.1 and <= 3.0.0

[ktbyers]

I vote that we don't pin napalm or netmiko to major versions (that we only say it has to be greater than X version). From a practical standpoint on both NAPALM and Netmiko we should know in advance if we broke compatibility and if we did break things accidentally then we would probably release a new version of NAPALM/Netmiko fairly quickly to fix it.

I also advocate that we 100% pin all of the dev dependencies as I hate the dev tooling breaking the build.

We should also pin pydantic to an exact version (given all of its instability) and I probably vote we 100% pin mypy as well.

[dmfigol]

I also advocate that we 100% pin all of the dev dependencies as I hate the dev tooling breaking the build.
I probably vote we 100% pin mypy as well.

again, everything is already pinned via the lock file, pinning it explicitly in toml file is not useful, because then it is harder to update using poetry update.

[ktbyers]

Good point...I keep forgetting the lock file mechanism.

Wouldn't we need to poetry update --no-dev on every Travis-CI run though?

In other words, shouldn't Travis-CI be identical to what an end-user doing an install would experience (as far as the non development dependencies). I could have easily missed it, but that is not what I was seeing?

Also wouldn't the nbval docker image need to use the lock file?

[dbarrosop]

I vote that we don't pin napalm or netmiko to major versions (that we only say it has to be greater than X version)

The greater than X is not really needed as poetry update is going to pick the latest available version that matches the rule and put it in the lockfile. The reasoning for having the major version is mostly to signal what a particular nornir version is supporting. If there was napalm3, that'd probably mean some backward compatibility change that might break things, then, by having nornir X saying it supports napalm=^2 we are saying we haven't tested napalm3 so if they want to use it, it should be at their own risk. It's also a bit of a moot point to discuss as most libraries don't change major versions in years and in the particular case of napalm/netmiko we can easily coordinate the releases if that was to happen.

Wouldn't we need to poetry update --no-dev on every Travis-CI run though?
Also wouldn't the nbval docker image need to use the lock file?

travis-ci is using the new docker mechanism I developed instead of tox which basically installs the software with poetry install (which uses the lockfile)

[ktbyers]

@dbarrosop

The greater than X is not really needed as poetry update is going to pick the
latest available version that matches the rule and put it in the lockfile. 

Is that right? When end-users install Nornir, they are only going to use PIP which will only use the pyproject.toml file. In other words, don't we need the TOML file to enforce minimum requirements?

I think this question would also still stand:

Wouldn't we need to poetry update --no-dev on every Travis-CI run though?

In other words, I would think one significant goal would be the Travis-CI to always match what end-users install with a fresh pip install for the production dependencies (not the dev dependencies).

[dmfigol]

@ktbyers

In other words, shouldn't Travis-CI be identical to what an end-user doing an install would experience

No, because travis build needs dev dependencies to run pytest, mypy, pylama, etc. Deploy stage just uses poetry publish which resolves constraints correctly for non-dev section, builds a wheel and publishes it.

@dbarrosop

The greater than X is not really needed as poetry update is going to pick the latest available version that matches the rule and put it in the lockfile

napalm and netmiko are non-dev dependencies, so what you mention is going to happen in dev (our) environment when we do poetry update, but our users will not see lockfile, they will get a package from PyPi with package constraints like netmiko >= 2.0.0, < 3.0.0 (which were specified in pyproject.toml file and converted by poetry)

Side note, I've had a number of problems in production with requests in setup.py because they specify urllib dependency upper boundary very aggressively.

That said, since David already mentioned that we are ok with releasing a new minor version of Nornir whenever a new major version of non-dev dependency is released, I don't want to keep fighting this. I think we can try and if in the future it proves to be too much overhead, we can re-evaluate it.

So once my latest review comments are addressed, it is LGTM.

[dmfigol]

Update: I was talking with Kirk privately and I think we came to an interesting idea.
How about adding poetry update --no-dev inside the dockerfile, so that every time the commit is pushed, the tests are run against the latest versions of prod dependencies satisfying package constraints – not what is in the lock file (which could lag a little)?
The only downside I could think of: our tests run locally using poetry run pytest [pylama, black, etc.] could have different results from make tests
The advantage is that at any push, we run tests against the latest versions of dependencies which users will get when they run pip install nornir. If something upstream breaks, we could find out about this quickly and pin that dependency with <=, release new nornir minor version, until upstream is resolved/investigated.

[dbarrosop]

but our users will not see lockfile

fixing

How about adding poetry update --no-dev inside the dockerfile

That doesn't work, unfortunately:

$ poetry update --no-dev
Updating dependencies
Resolving dependencies... (0.7s)


Package operations: 0 installs, 0 updates, 44 removals

  - Removing appdirs (1.4.3)
  - Removing atomicwrites (1.2.1)

If you do poetry install && poetry update --no-dev it uninstalls the development requirements and if you do poetry update --no-dev && poetry install it updates the development requirements, which is my main concern at this point.

I am merging this one after fixing the "greater than" requirements issue dmfigol highlighted. We can keep the discussion on the main issue and iterate on this although I don't think there is much that can be improved at this point :(

[ktbyers]

@dbarrosop Thanks for testing/checking on the --no-dev (obviously different behavior than I expected, but that is what I get for not testing it).

[dmfigol]

@dbarrosop This is merged, but could you still address my comment?

Reading from disk and parsing toml file twice.
This WA version = pkg_resources.get_distribution("nornir").version didn't work?

I've also raised python-poetry/poetry#829

[dbarrosop]

This WA version = pkg_resources.get_distribution("nornir").version didn't work?

That doesn't work in this particular case as RTD doesn't support poetry so I can't install the development version of the code to build the docs from the source. However, if you make it work somehow I will be glad to merge it (couldn't see any way of doing it myself)