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

Switch from pipenv to poetry #75

Merged
merged 3 commits into from
Jun 23, 2020
Merged

Conversation

nat-n
Copy link
Collaborator

@nat-n nat-n commented May 27, 2020

I believe I've faithfully reproduced the same project config with the exception of:

  • dropped dev dependency on rope, isort & flake, since they seem redundant or at least not used
  • poetry doesn't support dev scripts like pipenv (yet), so create a makefile instead. Could consider alternatives like invoke
  • Add pytest-cov, coverage may generally be abused as a metric, but I think it can be interesting to keep an eye on

Open question:

@nat-n nat-n marked this pull request as draft May 27, 2020 21:57
@nat-n nat-n force-pushed the add_poetry branch 2 times, most recently from f13c4ba to d86f697 Compare May 27, 2020 21:59
cetanu
cetanu previously approved these changes May 28, 2020
Copy link
Collaborator

@cetanu cetanu left a comment

Choose a reason for hiding this comment

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

Berry nice 🍓

Make is probably fine, especially with the current simplicity of the file. Invoke is good when the makefile grows in complexity.

Coverage is also good. We'll see what kind of coverage is there, and we can set some reasonable threshold depending on whether things are easily testable + important enough to be tested to that extent?

@boukeversteegh
Copy link
Collaborator

Pretty cool!

Apparently, its possible to use the virtual-envs created by poetry within PyCharm, so requirements don't need to be installed separately. That's great.

@nat-n nat-n force-pushed the add_poetry branch 18 times, most recently from 869c53d to 4e09900 Compare June 1, 2020 19:05
@nat-n nat-n force-pushed the add_poetry branch 3 times, most recently from 9aea9ca to ecf7eed Compare June 4, 2020 19:48
@nat-n nat-n marked this pull request as ready for review June 4, 2020 21:35
@nat-n nat-n force-pushed the add_poetry branch 3 times, most recently from 2dbda85 to fbee1a6 Compare June 5, 2020 21:31
@nat-n nat-n force-pushed the add_poetry branch 3 times, most recently from 36af2bb to afb8b7b Compare June 12, 2020 18:43
- dropped dev dependency on rope, isort & flake
- poetry doesn't support dev scripts like pipenv, so create a makefile instead
- Add pytest-cov
- Use tox for testing multiple python versions in CI
- Update README

Update ci workflow
@boukeversteegh
Copy link
Collaborator

Hi Nat~!

I've experimented today using this poetry branch. I ran into another issue #91 that first caused me to think there was a problem with this branch (protoc didn't give any output except exit code 1), but it turned out unrelated.

The biggest difference I've noticed using poetry is that installing the packages takes a very long time. Around 2 minutes. Apparently, this is a known issue of poetry, and a consequence of the design decision to prefer correctness over performance, and the way python packages are published does not allow for efficient dependency resolution.

I did not have any trouble that I had with pipenv before, although I was not able to install the dev dependencies using poetry. I did not find an option like --dev. How should we install those?

I'm a bit on the fence for poetry. It seems a more robust system, but the speed is really bad and we will also lose the taskrunner. Ofcourse you're working on poe 😉, but it does add a maintenance burden, whereas pipenv has a taskrunner included.

The main benefit of poetry seems more correct dependency resolution, but as far as I know, we don't have problems with dependencies not resolving correctly, at the moment.

I admittedly had trouble starting up with pipenv, but it was mainly because I didn't know the correct commands. Updating the docs could help there. Another issue we had was adding packages with pipenv without upgrading everything else. I believe that pinning the versions in the Pipfile could be a solution for that.

So in conclusions, I'm not sure we have strong arguments in favor of poetry, especially taking the downside of speed into consideration.

If we can find a way to make it "fast" (30 seconds? still slower than pip), I would go along and switch to poetry. There are some tricks like generating requirements.txt and installing it with pip, but in my experiment it gave some errors while installing that.

@nat-n
Copy link
Collaborator Author

nat-n commented Jun 13, 2020

@boukeversteegh The main motivation for this was to make the project more approachable for new contributors. The anecdata shows that pipenv is a little bit of an obstacle for newcomers to the project, and although documentation could help, poetry is still generally easier to pick up and work with.

I've never really noticed speed being an issue. My understanding is the poetry only takes the slow path when it finds ambiguities in the dependency graph. That said running poetry install is usually done only once during first setup of the environment so waiting 30s once doesn't strike me as a big deal. For comparison running pipenv install takes 25s for me, so it's not even a significant difference.

I'm not sure comparing to pip (which is still supported from the poetry style pyproject.toml) is the most meaningful comparison, since I don't see why it would be a common part of a development workflow, and of course it takes half the time (16s for me), because it's only doing half the work by ignoring dev dependencies.

There's no poetry install --dev because installing the dev dependencies into the development virtualenv is the default behaviour – which makes sense right?

To be clear using poetry vs good old fashion setup_tools has no impact on the installation speed of the resulting wheel or egg for the end user since poetry actually uses setup_tools for this process.

There's still the issue of having a handy task runner via the pipfile. I went with a makefile as a simple, familiar (sort of?) alternative solution. However since I started another nifty poetry oriented solution has popped up that can already do everything that pipfile scripts can do and more. Maybe we should just use that?

So contributors dont have to remember to run poetry install with `-E compiler`
```bash
pipenv run black .
```sh
make format
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe lets add the alternative poetry run black ., because we will ditch make pretty soon and it's not a tool most python developers will have

betterproto/tests/test_inputs.py Show resolved Hide resolved
@boukeversteegh boukeversteegh merged commit 5fb4b4b into danielgtaylor:master Jun 23, 2020
@nat-n nat-n deleted the add_poetry branch June 23, 2020 20:00
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