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

Docker build and wheel dist build fixes #223

Merged
merged 5 commits into from
Apr 21, 2024
Merged

Conversation

ashleysommer
Copy link
Contributor

@ashleysommer ashleysommer commented Apr 21, 2024

I found some inconsistencies and bugs while building Prez in a docker container for deployment on Azure cloud.

This is a collection of five small commits related to building the Prez bdist wheel, installing it in the Docker container, and building the Docker container.

1 Use arg/env variable for venv creation instead of hardcoded location. (It used the venv in other calls, just not in the venv creation command). Explicitly call the venv version of pip3 to install the wheel, so it is obvious the built prez module is being installed in the venv site_packages.

2 Don't copy the whole source tree into the final image, it already has the built prez module and all dependencies installed in the venv, it just needs main.py and supporting files.

3 Never include dist files in the docker env, this also ensures no dist files are included inside the sdist or wheel packages (poetry excludes packaging everything in gitignore).

4 Don't put Dockerfile inside its own Docker build context. Its not needed for the container built, and this allows incremental builds to work properly.

5 Put prez module's pyproject.toml inside the built prez module in the wheel package, this allows the config builder to find it when prez is installed as a system module. Looks like this is what #216 was trying to solve.
Modify the config builder to search inside the prez module for pyproject.toml if there is not one in the project top directory (enables the above functionality).
Also explicitly list all the files to include in a sdist, because should be different than the wheel bdist.

Explicitly call the venv version of pip3 to install the wheel, so it is obvious the built prez module is being installed in the venv site_packages.
… the built prez module and all dependencies installed in the venv, it just needs main.py and supporting files.
… files are included inside the sdist or wheel packages (poetry excludes packaging everything in gitignore).
…ded for the container built, and this allows incremental builds to work properly.
…e wheel package, this allows the config builder to find it when prez is installed as a system module.

Modify the config builder to search inside the prez module for pyproject.toml if there is not one in the project top directory (enables the above functionality).
Also explicitly list all the files to include in a sdist, because should be different than the wheel bdist.
Copy link
Collaborator

@edmondchuc edmondchuc left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for that Ashley.

Nice, I didn't know pyproject.toml had to be explicitly included like that. It's probably why the last few releases had the version missing since it couldn't find the file.

@edmondchuc edmondchuc merged commit 0437c75 into main Apr 21, 2024
2 checks passed
@edmondchuc edmondchuc deleted the docker_build_fixes branch April 21, 2024 04:15
@lalewis1
Copy link
Collaborator

lalewis1 commented Apr 21, 2024

@ashleysommer and @edmondchuc. Hey guys, I just pulled these changes and when I try to run poetry install Poetry is complaining that:

The Poetry configuration is invalid:

  • data.packages[1] must not contain {'to'} properties

Just checking if this is something that is happening to you both as well or if it is just me?

Python version == 3.11
Poetry version == 1.7.1

Note: I checked the poetry docs and I can't see why it would be unhappy with the 'to' declaration in pyproject.toml, Only thing I can think of is that maybe it has to have a 'from' declaration as well.

@ashleysommer
Copy link
Contributor Author

ashleysommer commented Apr 21, 2024

Hi @lalewis1 I'm on Poetry version v1.8.1, and it works as expected in my testing.

I just checked the documentation, and I didn't realize that is a feature that was recently introduced in Poetry v1.8.0
https://python-poetry.org/docs/

More specifically, I found its a feature of poetry-core v1.9.0+ python-poetry/poetry-core#672

It's not in the docs for Poetry v1.7, so it must've been added after that. I thought that was a basic packaging feature that has been around for years.

Do you guys have a policy on supported Poetry version? When building Prez in a docker container its not an issue because latest Poetry is always used inside the container. But is it reasonable to expect developers on the code to always have the latest Poetry version, or should we maintain some kind of backwards compatibility?

@lalewis1
Copy link
Collaborator

I think it would be fine to have the dependency on the latest version. It would seem there is no way to add the requirement in pyproject.toml (python-poetry/poetry#6088), so perhaps the best thing will be to add a note to the readme.

@lalewis1
Copy link
Collaborator

I just upgrade poetry to 1.8.2 and poetry-core 1.9.0 and it is now working as expected for me as well.

Thanks for the quick investigation!

@ashleysommer
Copy link
Contributor Author

Actually, you can specify the required poetry-core version used for the build backend here:

requires = ["poetry-core>=1.0.0"]

Perhaps its worth bumping that to v1.9 now, then using that as the baseline minimum-supported poetry-core.

@lalewis1
Copy link
Collaborator

yes good idea. I'll do it now.

lalewis1 added a commit that referenced this pull request Apr 21, 2024
Changes to packaging declarations in PR #223 introduced a dependency on a feature in this version of poetry-core.

Specifically the use of a 'to=' statement in the packages array.
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