pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.35k stars 640 forks source link

Transitive excludes on python requirements do not work for external libraries #11324

Open jsirois opened 3 years ago

jsirois commented 3 years ago

Transitive dependency excludes do not work for packages defined in external librarie's requirements.txt

Given this in 3rdparty/python/BUILD:

python_requirement_library(
   name="somelib",
   requirements=["somelib==1.0.0"]
)

python_requirement_library(
   name="someotherlib",
   requirements=["someotherlib==2.0.0"]
)

where somelib's requirements.txt includes someotherlib as a requirement, i.e.: requirements.txt of somelib:

someotherlib==2.0.0

and this in src/mylib/BUILD

python_library(name="base")

python_awslambda(
    name="awslambda",
    dependencies=[
        ":base",
        "3rdparty/python:somelib",
        "!!3rdparty/python:someotherlib",
    ],
    runtime="python3.8",
    handler="lambda_function:lambda_handler",
)

and running ./pants package src/mylib:awslambda, the resulting package still includes someotherlib (most likely because somelib needs it in its own requirements.txt, and pants does not look like it's aware of that.).

For my use case, I'm trying to use an external library which makes use of boto3 and botocore: aws-secretsmanager-caching, and I need the lambda package to not contain the boto3and botocorepackages, because they take too much space in the lambda package and the lambda runtime already includes them out of the box.

The current workaround I have is to clone the aws-secretsmanager-cachinglibrary, convert it to a pants accepted build folder using BUILD files, and then the exclude works as expected, however this is not ideal since it copies source code from another library into my project.

Any better way to do this ? Is this a feature that's supposed to be supported by pants ? Thanks for your feedback.

jsirois commented 3 years ago

Moved this issue here from https://github.com/pantsbuild/pex/issues/1146 for @GLeurquin.

GLeurquin commented 3 years ago

Thanks @jsirois, indeed had the wrong tab opened (pex instead of pants).

I'll add that the version of pants I'm using is: 2.1.0

Eric-Arellano commented 3 years ago

Hm, John, I think this is actually a Pex issue.

Is this a feature that's supposed to be supported by pants ?

Not in the current form, no. The current transitive excludes code deals in the abstraction level of targets. You can say to ignore a python_requirement_library, which removes it from the inputs to the command pex -o app.pex req1 req2 req3.

But what another python_requirement_library's dependencies are—as expressed in its setup.py—is completely out of the domain of Pants, and should be. Pants has no notion of what a top-level requirement's transitive deps are.

To support this, Pants would need to do something like pex -o app.pex req1 req2 req3 --exclude req4. @jsirois, initial thoughts on Pex having an --exclude feature?

jsirois commented 3 years ago

Hm, John, I think this is actually a Pex issue.

I don't think so. The python community does not support requirement excludes in any tool I'm aware of or any relevant PEP. If Pants wants to extend its exclude mechanism for local code to remote code, it will need to work around ecosystems that do not support this for 3rdparty deps. So, for the jvm it can leverage maven excludes, for Python it cannot.

Some relevant issues in the python community asking for this feature from dependency resolvers: https://github.com/pypa/pip/issues/3090#issuecomment-290439264 https://github.com/python-poetry/poetry/issues/697#issuecomment-744235786

To support this, Pants would need to do something like pex -o app.pex req1 req2 req3 --exclude req4. @jsirois, initial thoughts on Pex having an --exclude feature?

Pex should definitely not support this.

Eric-Arellano commented 3 years ago

Hm, John, I think this is actually a Pex issue.

I don't think so.

+1, this then is really a pip issue.

Pex should definitely not support this.

Likewise, I don't think Pants should support this without Pex supporting it, which requires pip / Python ecosystem supporting it.

I'm going to close as out of scope @GLeurquin.

cburroughs commented 4 months ago

Reopening due to Pex support.