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

Including Poetry files for Dependency Management #264

Closed
wants to merge 3 commits into from
Closed

Including Poetry files for Dependency Management #264

wants to merge 3 commits into from

Conversation

vedosis
Copy link

@vedosis vedosis commented May 31, 2021

Added required files, imported existing requirements.txt and requirements.dev.txt dependencies.

https://python-poetry.org/docs/ is a PEP 582 compatible dependency backend. I chose it because pipenv (personal favorite) doesn't seem to be moving forward at the moment, pdm doesn't play well with some datascience libs (pycocotools in particular, scipy in general), and poetry generates pip compatible requirements.txt files for "production."

In theory, we shouldn't need anything extra for the docker environment if we used the poetry output to fix the dependency tree in a point in time. Additionally, it'll be easier to upgrade ALL THE THINGS by using a more diverse versioning syntax (https://python-poetry.org/docs/dependency-specification/)

poetry install to setup a dev environment
poetry install --no-dev to setup a prod environment

To install to root system (Docker)

poetry config settings.virtualenvs.create false
poetry install --no-dev

@derneuere
Copy link
Member

Hey @vedosis, thanks for your contribution. This does sound like a better solution than just using a regular requirements file.
When learning about poetry I found a couple of questions regarding LibrePhotos:

At the moment we install pytorch and pyvision in the dockerfile and install script. Would it be possible to add this dependency to poetry?

https://github.com/LibrePhotos/librephotos-docker/blob/7713e6ffecddb5542c7b8cc767a32703887b0b85/backend/Dockerfile#L16

We also build dlib from source, and I am not sure how to exclude them when building the requirement file. Or do we then have to spin up a private repository to add it to the poetry file via the url tag?

https://github.com/LibrePhotos/librephotos-docker/blob/7713e6ffecddb5542c7b8cc767a32703887b0b85/backend/Dockerfile#L19-L26

@vedosis
Copy link
Author

vedosis commented Jun 2, 2021

What a dizzying little rabbit hole you found for me. Like seriously. People seem to have strong opinions in this area.. Long story short, "let me see if I can." I can use the url option, but there's some issues with cross dependencies. i.e. torch:1.8.1+cpu does not provide the 1.8.1 requirement from torchvision:0.9.1 because reasons? 🤷🏻‍♂️

Let me play with this a little while and get back to you.

@vedosis
Copy link
Author

vedosis commented Jun 26, 2021

Just bringing this back up here. I've filed a pull request upstream to poetry which fixes the most immediate problem. If they approve it, we can move torch and torchvision into the spec (per my updated pull request).

Still thinking about and working on dlib.

@vedosis
Copy link
Author

vedosis commented Jun 26, 2021

Follow up question @derneuere, is there a reason we're building from src on dlib? There's a pypi package available https://pypi.org/project/dlib/#files.

@sonarcloud
Copy link

sonarcloud bot commented Jun 27, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derneuere
Copy link
Member

Awesome work!

Dlib has to be compiled AFAIK. The dlib wheel from pip will also compile it, but it does not have an option to compile it explicitly to CPUs that don't have AVX/SSE. The GitHub action build servers all have new CPUs that have these instruction sets, so the wheel will just compile it for AVX.

This leads to an error where, for example, LibrePhotos will just constantly reboot on old xeon processors.

Maybe I missed something in the documentation 😅

@vedosis vedosis closed this Nov 21, 2021
AnkurPrabhu pushed a commit to AnkurPrabhu/librephotos that referenced this pull request Oct 22, 2023
…lugin-react-7.x

Update dependency eslint-plugin-react to v7.33.1
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.

2 participants