search

paketo-buildpacks / poetry-install

Apache License 2.0
1 stars 7 forks source link

[Question ❓] How to install only "root/run" dependencies and ignore other groups dependencies #180

Open Mehdi-H opened 1 year ago

Mehdi-H commented 1 year ago

General summary of the issue

I am suprised that running "pack build" with paketo-buildpacks/poetry-install installs all the dependencies described in my pyproject.toml file.

For production, I only want to install run dependencies (in [tool.poetry.dependencies] section), dependencies in other groups (such as [tool.poetry.group.dev.dependencies]) are not necessary and make the built image heavier than necessary

Expected Behavior

When running pack build [my_image] --builder paketobuildpacks/builder:base (using paketo-buildpacks/poetry-install in Detection phase) I expect paketo-buildpacks/poetry-install to run the following command :

POETRY_CACHE_DIR=/layers/paketo-buildpacks_poetry-install/cache POETRY_VIRTUALENVS_PATH=/layers/paketo-buildpacks_poetry-install/poetry-venv poetry install --only main

Having the following dependencies in pyproject.toml, I expect only fastapi and uvicorn to be installed, without pytest ✅ : 

[tool.poetry.dependencies]
python = "^3.11"
fastapi = "^0.100.0"
uvicorn = {extras = ["standard"], version = "^0.23.1"}

[tool.poetry.group.dev.dependencies]
pytest = "^7.4.0"

Current Behavior

When running pack build [my_image] --builder paketobuildpacks/builder:base (using paketo-buildpacks/poetry-install in Detection phase), I see in the log that the following command is run :

POETRY_CACHE_DIR=/layers/paketo-buildpacks_poetry-install/cache POETRY_VIRTUALENVS_PATH=/layers/paketo-buildpacks_poetry-install/poetry-venv poetry install

Having the following dependencies in pyproject.toml, pytest is installed 😢 : 

[tool.poetry.dependencies]
python = "^3.11"
fastapi = "^0.100.0"
uvicorn = {extras = ["standard"], version = "^0.23.1"}

[tool.poetry.group.dev.dependencies]
pytest = "^7.4.0"

I see the following logs in the console : 

Paketo Buildpack for Poetry Install 0.3.16
  Executing build process
    Running 'POETRY_CACHE_DIR=/layers/paketo-buildpacks_poetry-install/cache POETRY_VIRTUALENVS_PATH=/layers/paketo-buildpacks_poetry-install/poetry-venv poetry install'
      Installing dependencies from lock file

      Package operations: 4 installs, 0 updates, 0 removals

        • Installing iniconfig (2.0.0)
        • Installing packaging (23.1)
        • Installing pluggy (1.2.0)
        • Installing pytest (7.4.0)

Question

I don't see any configuration or environment variable to add arguments to "poetry install" command in the README.md file, is there something that I missed ?

Steps to Reproduce

Tell me if this is necessary and I can setup a repository with reproducible steps, as it can take me some time to do it. I'll do it happily :)

Motivations

How has this issue affected you? What are you trying to accomplish? What is the impact? Providing context helps us come up with a solution that is most useful in the real world.

In the current situation, having poetry installing all dependencies, even the ones I do not need is a no-go for any of my projects in production.

robdimsdale commented 1 year ago

In the current situation, having poetry installing all dependencies, even the ones I do not need is a no-go for any of my projects in production.

Makes total sense. You're on the leading edge! Not many folks are using paketo buildpacks + poetry in production yet.

I can reproduce this, and I agree we should provide a mechanism to only install production dependencies. I was hoping that we could leverage environment variables to configure which dependencies to install (similar to NODE_ENV or RAILS_ENV) but it seems that environment variable config is only for poetry itself, not for determining what flags to pass to poetry install.

So it seems we'll have to invent an API for this. I think it would look something like providing the environment variable BP_POETRY_INSTALL_ONLY=<comma-separated-list>, and we'd essentially pass that through to the poetry install command.

Is this something you're interested in contributing? We can lay out the changes necessary if so.

Mehdi-H commented 1 year ago

Hello @robdimsdale, thanks for the quick answer !

I don't know paketo, buildpacks nor Go very well but I can take a look at it and try to make a pull request :)

I will most certainly ask you some questions in this issue after taking a look at the code !

robdimsdale commented 1 year ago

Awesome!

So I think you're going to want to start with this line in install_process.go:

args := []string{"install"}

I think we're going to want to expand that to read from the environment variable BP_POETRY_INSTALL_ONLY, similar to how we do this in the pip-install buildpack:

if destPath, exists := os.LookupEnv("BP_PIP_DEST_PATH"); exists {
  vendorDir = filepath.Join(workingDir, destPath)
}

We should add unit tests for this in install_process_test.go - again you can crib the structure from the pip-install buildpack.

Finally we should probably add an integration test. You could probably copy integration/default_test.go (and the associated fixture in integration/testdata/) and make a new test called something like integration/dependency_groups.go. That way we can expand the test in the future if we also support flags like --with/--without, or --only-root, etc.

Mehdi-H commented 1 year ago

Hello @robdimsdale ,

I took a look at Poetry documentation for the install command, and surprisingly, it is not possible to install only "root/run" dependencies with $> poetry install --only root. I have the following error when running this locally : 

$>poetry install --with root;

Group(s) not found: root (via --with)

It seems, the only way to achieve this is to run poetry install with another option : --only-root.

Before starting to implement tests and code, I wanted to check with you the expected behavior for this pull request :

What do you think ?

Also, maybe the env variable should be named BP_POETRY_INSTALL_WITH instead of BP_POETRY_INSTALL_ONLY if we use --only option

robdimsdale commented 1 year ago

Oh, sorry, I thought you wanted to install --only main. Are you sure you want --only-root? From my reading of the documentation that will ignore all other dependencies:

If you want to install the project root, and no other dependencies, you can use the --only-root option.

https://python-poetry.org/docs/managing-dependencies/#dependency-groups

Mehdi-H commented 1 year ago

Oh, I learned something !

As the "main/root" group name is not documented well in Poetry docs, I thought it was not possible to install only the main group with --with parameter/

I'll go with the following tests suite :

What do you think ?

robdimsdale commented 1 year ago

Yup that sounds good, thanks!

Technically it's a breaking change to default to --with main but I think that's reasonable, given we default to production-like environments in other buildpacks (e.g. RAILS_ENV defaults to production).

Also Cloud Native Buildpacks doesn't have any mechanism for running tests, or toolchain commands (like code coverage, etc) so I think defaulting to main shouldn't break anyone unless they're using the resultant image in a way other than docker run <my-app>

Mehdi-H commented 1 year ago

Hi @robdimsdale ,

I have started an implementation, with unit tests passing, in this fork : https://github.com/Mehdi-H/poetry-install/commit/84c30e12b08d037b240d75784d9eb73b1650d0e7

I have trouble with the integration tests : I managed to run them, but they are failing as the default behavior is breaking

During the integration tests, the build ends up with this error : 

            [builder]   Executing build process
            [builder]     Running 'POETRY_CACHE_DIR=/layers/paketo-buildpacks_poetry-install/cache POETRY_VIRTUALENVS_PATH=/layers/paketo-buildpacks_poetry-install/poetry-venv poetry install --with main'
            [builder]       
            [builder]       The command "install --with main" does not exist.
            [builder] poetry install failed:
            [builder] error: exit status 1
            [builder] ERROR: failed to build: exit status 1
            ERROR: failed to build: executing lifecycle. This may be the result of using an untrusted builder: failed with status code: 51

I am a bit lost as to what assertion in the integration tests is failing exactly, a global repo search on "does not exist" gets 0 result.

Any idea where to look ?

robdimsdale commented 1 year ago

Thanks for getting started with this.

The issue is that the args need to be separate strings in an array, not space-separated. What's happening in your code currently is the following (quotes added to make it clear that's a single argument):

poetry 'install --with main'

When really what you need is:

poetry 'install' '--with' 'main'

If you change:

args = []string{"install --with " + group}

to:

args = []string{"install", "--with", group}

That should resolve your issue.

robdimsdale commented 1 year ago

Otherwise this PR looks like it's heading in the right direction!

Mehdi-H commented 1 year ago

Hello @robdimsdale ,

Unit and integration tests are green, can you take a look at this pull request ? - #182

I'll take any feedback !

EDIT: I see that the PR pipeline is red, but I don't think I can fix it on my own

binomaiheu commented 1 year ago

I would welcome this as well, thanks for the fix @Mehdi-H ! Any update on integration ?

Mehdi-H commented 1 year ago

Hello @binomaiheu and @robdimsdale, sorry for the inactivity, I did not have access to the internet for the past month

I see that I have to

I will try to take a look at all this in the next few days

radoshi commented 7 months ago

We got hit by this as well. I'd like to add a potential work around:

In pyproject.toml you can mark all groups except your main one as optional. This increases the headache for people trying to install some basic set of groups together for dev, but fixes the container building because pack build now picks up only the non-optional groups.

Ref: https://python-poetry.org/docs/managing-dependencies/#optional-groups

funkindy commented 1 month ago

Any updates on this? Got hit also.