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

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

Open
Mehdi-H opened this issue Jul 25, 2023 · 15 comments · May be fixed by #424
Open

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

Mehdi-H opened this issue Jul 25, 2023 · 15 comments · May be fixed by #424
Labels
good first issue A good first issue to get started with hacktoberfest Hacktoberfest eligible

Comments

@Mehdi-H
Copy link

Mehdi-H commented Jul 25, 2023

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
Copy link
Member

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.

@robdimsdale robdimsdale added the good first issue A good first issue to get started with label Jul 25, 2023
@Mehdi-H
Copy link
Author

Mehdi-H commented Jul 26, 2023

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
Copy link
Member

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
Copy link
Author

Mehdi-H commented Jul 26, 2023

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 :

  • [⚠️ Breaking change] New default behavior, if BP_POETRY_INSTALL_ONLY env variable is not set, it should install only root dependencies with poetry install --only-root (was poetry install)
  • [🆕 ] New feature: if BP_POETRY_INSTALL_ONLY env variable is set, value will be appended to poetry install --with.
    • example : with BP_POETRY_INSTALL_ONLY=dev, poetry install --with dev will be executed, installing root group and dev group
    • example: with BP_POETRY_INSTALL_ONLY=dev, lint, poetry install --with dev,lint will be executed, installing root group, dev group and lint dependencies group

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
Copy link
Member

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
Copy link
Author

Mehdi-H commented Jul 28, 2023

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 :

  • Given BP_POETRY_INSTALL_ONLY is not set, poetry install --with main should be executed
  • Given BP_POETRY_INSTALL_ONLY=dev, poetry install --with dev should be executed
  • Given BP_POETRY_INSTALL_ONLY=main,dev, poetry install --with main,dev should be executed

What do you think ?

@robdimsdale
Copy link
Member

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
Copy link
Author

Mehdi-H commented Jul 28, 2023

Hi @robdimsdale ,

I have started an implementation, with unit tests passing, in this fork : Mehdi-H@84c30e1

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
Copy link
Member

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
Copy link
Member

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

@Mehdi-H
Copy link
Author

Mehdi-H commented Jul 30, 2023

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
Copy link

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

@Mehdi-H
Copy link
Author

Mehdi-H commented Sep 7, 2023

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

  • update the branch, as the main branch has evolved since I opened this PR,
  • apply your code review feedback on the code,
  • and take a look at this "CLA" document to sign

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

@radoshi
Copy link

radoshi commented Apr 19, 2024

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

@TheSuperiorStanislav TheSuperiorStanislav linked a pull request Sep 5, 2024 that will close this issue
5 tasks
@funkindy
Copy link

funkindy commented Oct 1, 2024

Any updates on this? Got hit also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue A good first issue to get started with hacktoberfest Hacktoberfest eligible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants