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

[WIP] Use poetry #715

Closed
wants to merge 11 commits into from
Closed

Conversation

bartekpacia
Copy link
Contributor

Description

Migrated to Poetry as our default dependency management tool. It's simple, yet powerful.

  • added pyproject.toml
  • removed requirements.txt

Docker is currently broken I'm not sure how to fix this!

mentorship-backend(use_poetry) ➜ docker build -t mentorship-backend:latest .
Sending build context to Docker daemon  673.3kB
Step 1/7 : FROM python:3.7
 ---> fbf9f709ca9f
Step 2/7 : COPY ./pyproject.toml /dockerBuild/pyproject.toml
 ---> Using cache
 ---> 9e4608c4655e
Step 3/7 : WORKDIR /dockerBuild
 ---> Using cache
 ---> ad1effb304dc
Step 4/7 : RUN curl -sSL https://raw.githubusercontent.com/python-poetry/poetry/master/get-poetry.py | python
 ---> Using cache
 ---> dc3855305db0
Step 5/7 : RUN poetry install
 ---> Running in eae19bec558e
/bin/sh: 1: poetry: not found
The command '/bin/sh -c poetry install' returned a non-zero code: 127

fixes #691

Type of Change:

  • Code
  • Documentation

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Run the test suite, all tests pass.

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged
  • Update Postman API at /docs folder
  • Update Swagger documentation and the exported file at /docs folder
  • Update requirements.txt Not anymore! Updated pyproject.toml haha

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@bartekpacia
Copy link
Contributor Author

Currently poetry run start and poetry run test are broken. I'm looking for a solution.

@isabelcosta isabelcosta changed the title Use poetry [WIP] Use poetry Aug 5, 2020
Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

Couple of comments!

I wrote this a couple of months ago and forgot to update this, but feel free to take some pointers from here if you are struggling with the documentation.

https://github.com/SanketDG/djurl/tree/master/modern-django-tooling#use-poetry-for-dependency-management-or-pip-tools

"Anita Borg",
"Poetry",
"Flask",
"Women in Technology"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a hard time digesting poetry's suitable for just dependency management when using with Django, because of the way it enforces package information, which is largely redundant here for the most part.

This is why my mind says to go with pip-tools when working with Django, even though I myself really like Poetry! 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have much experience with Django so you're probably right😁Idk how the situation looks like when using Flask

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I mean both Django/Flask, apologies, anything that is not a Python package, basically.

urllib3 = "^1.22"
wcwidth = "^0.1.7"
websocket-client = "^0.48.0"
Werkzeug = "^0.15.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need all of the pacakages here? I am pretty sure some of them are actually sub deps of other packages, that have creep'd into here (more reason for doing this 🙂 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go through them and remove the subdeps:)

@@ -1,74 +0,0 @@
alembic==1.4.2
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need to exacly remove this file, this file can still be used, especially with Docker!

Accurate dependency resolution is a hard problem, and poetry does it really well, so it takes a toll on time required for doing poetry install, instead we can make sure to always export our dependencies to a requirements.txt file, which would contain pinned versions and hash checks.

We can then re-use this file for people who absolutely still cannot use poetry for some reason, and we should also use this file for things like CI and deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would simply auto-generating this file (during a e.g CI) be a good solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the dev would generate and commit it (there are even git commit hooks for that) whenever the dev adds/updates/removes a package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But why? If it's the only purpose would be to be utilized by CI, then I think it's better to hide it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason is because it is simpler, there are enough issues on the Poetry repo about the dependency resolution taking ages sometimes, because it's a costly operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was taking a shower when I realized that my question (above) is stupid and the reason is obvious. I agree totally with this.

@@ -0,0 +1,11 @@
# This is a temporary workaround till Poetry supports scripts, see
# https://github.com/sdispater/poetry/issues/241.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish this was done soon enough with poetry, but they have been prioritizing with other things.



def test():
check_call(["python", "-m", "unittest", "discover", "tests"])
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also add scripts for linting / autoformatting 🙂

3. Install all the dependencies in `requirements.txt` file:
`pip install -r requirements.txt`
3. Activate the virtual environment (auto-crated by Poetry):
`poetry shell`
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also document briefly about poetry run or poetry shell, that would be helpful for people who still use virtualenv driven dev, so to speak 🙂

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 7, 2020

@isabelcosta @SanketDG Please review the newest changes:)

  • fixed scripts not working
  • remove all packages that I identified to be peer dependencies of other packages

@bartekpacia
Copy link
Contributor Author

I also tested this with Python 3.8 and all tests pass, so I think it's a great idea to bump the recommended Python version to 3.8

@SanketDG
Copy link
Contributor

SanketDG commented Aug 18, 2020

python-poetry/poetry#366

Dropping this here, seems useful

Edit (for caching): python-poetry/poetry#366 (comment)

@bartekpacia
Copy link
Contributor Author

Thank you @SanketDG. I'm busy right now but will surely find some time in the future to finish this PR

@bartekpacia
Copy link
Contributor Author

I'd like to finish this issue (hey, it's Hacktoberfest!) as soon as I get my laptop back.

Just pinging so it doesn't get closed:)

@isabelcosta
Copy link
Member

@bartekpacia ack

@bartekpacia bartekpacia mentioned this pull request Oct 18, 2020
13 tasks
@bartekpacia
Copy link
Contributor Author

a new PR #914 is up, so I'm closing this:)
Big thank you to everybody who commented and helped me here!

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.

Improve way of managing python dependencies
3 participants