pantsbuild / pants

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

`poetry_requirements` includes `[tool.poetry.dev-dependencies]` dependencies #17554

Open ioangatop opened 1 year ago

ioangatop commented 1 year ago

Description When producing distributions using the command ./pants package ::, where each BUILD of a library installs requirements with poetry_requirements, installs both [tool.poetry.dependencies] and [tool.poetry.dev-dependencies], which should not happen, at least by default.

Pants version 2.14

OS MacOS

Additional info Here some sample project files;

# BUILD file

poetry_requirements(name="lib-reqs")

python_sources(
    name="lib",
    sources=["libs/lib/**/*.py"],
    dependencies=[":lib-reqs"],
)

python_distribution(
    name="lib-dist",
    dependencies=[":lib"],
    provides=python_artifact(name="lib"),
    sdist=False,
)
[build-system]
requires = ["poetry-core>=1.0.0"]
build-backend = "poetry.core.masonry.api"

[tool.poetry]
name = "lib"
version = "0.0.1"
description = "lib purpose."

[tool.poetry.dependencies]
python = ">=3.8"
numpy = "^1"

[tool.poetry.dev-dependencies]
pytest = "^7.1.2"
black = "^22.3.0"
flake8 = "^4.0.1"
benjyw commented 1 year ago

Ah, the issue is that you are manually forcing a dependency on the entire poetry requirements set, because of dependencies=[":lib-reqs"],. Generally you don't need that - Pants uses dependency inference to pick out just the requirements you actually use.

If you do need to force one specific one, you can use dependencies=[":lib-reqs#numpy"], or whatever, but this is only necessary if dep inference fails to detect a dep (say because there is no corresponding import statement, and the code is loaded by name at runtime).

benjyw commented 1 year ago

I've removed the bug label, since this is working as intended, but I'll wait until you confirm that this solves the problem before closing the ticket.

kaos commented 1 year ago

but this is only necessary if dep inference fails to detect a dep (say because there is no corresponding import statement)

and in case it fails in face of there being an import statement, it's likely a mismatch between the import name and the distribution name (see doc), and for that there is the module_mapping field on the poetry_requirements target. (see doc)

As an additional rare example it might be curious to note that at some times, the missing requirement may be a transitive dependency of the thing you are importing, say if you use django.contrib.postgres that uses psycopg2, so in order to pull that in automatically you'd need a little config trick:

# Any of the *_requirements targets really..
poetry_requirements(
python_requirements(

    module_mapping={
        "psycopg2": [
            "psycopg2",
            # This is for dependency inference only, when we see an import for
            # `django.contrib.postgres` we should infer a dependency on `psycopg2`.
            "django.contrib.postgres",
        ],
    }
)
benjyw commented 1 year ago

I believe that trick is only necessary when the dep doesn't correctly declare its own deps though?

kaos commented 1 year ago

I believe that trick is only necessary when the dep doesn't correctly declare its own deps though?

In general, yes. The trick here I believe is that the dep is optional, you only want the psycopg2 dep if you actually use the postgres contrib module.. and for this case, I'm not entirely sure, but I think it may be part of the main Django dist..

kaos commented 1 year ago

but as stated, it should be rare.. ;)

ioangatop commented 1 year ago

Ah, the issue is that you are manually forcing a dependency on the entire poetry requirements set, because of dependencies=[":lib-reqs"],. Generally you don't need that - Pants uses dependency inference to pick out just the requirements you actually use.

Hi @benjyw thank you for the feedback! unfortunately, removing the dependencies=[":lib-reqs"] does not work (in test for example it can not find the packages) :/

In general I would like to avoid to use explicit packages in BUILD files (like dependencies=[":lib-reqs#numpy"]), because I would like to have a single entry point

As an additional rare example it might be curious to note that at some times, the missing requirement may be a transitive dependency of the thing you are importing, say if you use django.contrib.postgres that uses psycopg2, so in order to pull that in automatically you'd need a little config trick:

Thanks for the reply and the tip @kaos!

benjyw commented 1 year ago

Hi, can you clarify what you mean by "single entry point"? Not sure I understand how that impacts anything. To clarify, you should not need explicit packages in BUILD files, unless dependency inference fails to infer a dependency automatically. Generally speaking, if you remove that dependencies= line, things should just work...

jsirois commented 1 year ago

Just to perturb the bug-goggles, this looks like it could be another case of dependencies on python_distribution targets leading to unexpected results in the dependencies department.

benjyw commented 1 year ago

Just to perturb the bug-goggles, this looks like it could be another case of dependencies on python_distribution targets leading to unexpected results in the dependencies department.

I don't think that's the case here though. There are no such deps in the example given.

jsirois commented 1 year ago

There was mention by the OP in a comment about "in test for example".

ioangatop commented 1 year ago

@benjyw does it make sense to have an argument in poetry_requirements of which poetry group to include? It could be by default "all", but you can change it to "dev", "core" etc.

benjyw commented 1 year ago

Possibly, but first I'd like to get to the bottom of why you need that explicit dep to begin with...

benjyw commented 1 year ago

See my question above.

ioangatop commented 1 year ago

See my question above.

Yes, I'll try to find time to create a small repo!

benjyw commented 1 year ago

Hi @ioangatop - is this issue still relevant? We expect that you shouldn't need that explicit dep, but if you do it would be good to find out why.