Skip to content

Switch to poetry#1083

Merged
subdavis merged 8 commits into
mainfrom
housekeeping/poetry
Dec 8, 2021
Merged

Switch to poetry#1083
subdavis merged 8 commits into
mainfrom
housekeeping/poetry

Conversation

@subdavis
Copy link
Copy Markdown
Contributor

@subdavis subdavis commented Nov 30, 2021

Important

Requires update to compose file in prod.

Summary of changes

See also: https://www.notion.so/2ad1e1cce9204b37a6e406d76bb1e705?v=edef249eb5144b0a860528c90d77d15f&p=b452ad4d816a4cdda3726847c74fc21f

  • Switch to poetry dependency management, abandon setuptools
  • Check in new poetry.lock

Required several changes to the docker image

  • Abandon girder/girder official docker image, it wasn't providing value and was based on node:12, so we were bundling outdated dead dependencies. Switch to python:3.7-slim-buster as the final stage and buster as the build stage. slim-buster is a stripped down buster, which closely resembles our ubuntu developement environments and is more "familar" than the unfriendly alpine linux image. Apline is great for statically compiled languages like Go, but for us, the target environment is too unusual (musl/glibc dynamic linking is the main issue)
  • Replace tini init system with docker-builtin init: true (new docker feature since we first wrote this)
  • Redo girder image dockerfile, switch to 3 stage build. Reduces image size from 1.2GB to about 430MB.
  • Redo worker image, switch to 2 stage, reduces size from 20GBish to 16.7GB (base image + 300MB)
  • Remove girder dev entrypoint script that used to run pip install -e since poetry is editable-by-default
  • Add additional command support to startup script, and use --mode production or --dev to switch between dev an prod modes when running with ldc up -d or ldc dev up girder

I see no downsides to this change, it has simplified the docker build and made the development mode hot reload easier to use

ship

@subdavis subdavis marked this pull request as ready for review December 1, 2021 15:34
@subdavis subdavis force-pushed the housekeeping/poetry branch from da9eae2 to cdce36a Compare December 1, 2021 16:16
Comment thread docker/girder_worker.Dockerfile Outdated
Comment on lines +22 to +26
RUN poetry install --no-root --no-dev

# Copy full source code and install again
COPY server/ /home/server/
RUN poetry install --no-dev
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a question here. If you have the .toml and the .lock in the folder already with the full source code won't it just use them by default? I'm curious as to why it requires a second install?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's done for the sake of build cache. Most of the time you're changing code, not dependencies. When you ldc build girder you can just skip the dependency installation and get right to linking.

BryonLewis
BryonLewis previously approved these changes Dec 2, 2021
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

I've pulled, ran through some basic testing with uploading/importing and running pipelines. Also ran the integration test succesfully.
I think I have a base understanding of what the changes are doing and how they are affecting the install.

One final question/comment since we are using the master branch of poetry, should we switch the get-poetry.py to install-poetry.py considering that poetry 1.2 will be dropping get-poetry.py? We may end up in the same instance in which an update to our package dependency tool meant to fix issues by locking dependencies just refuses to build one day soon?
ref: https://python-poetry.org/docs/master/

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Dec 2, 2021

Yes, good catch. I didn't realize that.

I will also "pin" the poetry version using POETRY_VERSION=1.2.0

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Dec 3, 2021

Turns out that the new installer changed the install behavior in a subtle way that really impacts docker. It used to just install poetry on the system, but now the poetry install goes to some particular path. the behavior of virtualenvs.create configuration also changed to use PATH/venv/ instead of PATH.

  • Poetry is designed to be installed alone, in its own virtual environment, or on the system. Poetry cannot be installed into the same local install path as any of the things it manages becuase poetry's dependency versions can interfere with a package's dependencies.
  • However, we do it anyway for ~~ reasons ~~

I've been using docker for years, but I'm really still learning a lot about how to organize stuff. Multistage builds are a powerful way to simplify your code and shrink your images at the same time.

Now, instead of hacking the internals of site-packages, which was wrong, I'm copying the whole virtual env with the guarantee that the venv includes only my dive_server installation and nothing else. The poetry installation is left behind.

  • source now goes in /opt/dive/src which is more unix-conventional than /home/something
  • the python env goes in /opt/dive/local, which is the same as /opt/noaa/viame in structure.

@subdavis subdavis requested a review from BryonLewis December 7, 2021 15:16
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

pulled, built and ran integration testing again, looks good.

@subdavis subdavis merged commit fbf5b1f into main Dec 8, 2021
@subdavis subdavis deleted the housekeeping/poetry branch December 8, 2021 01:04
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