add entry_points field on python_distribution #11872

kaos · 2021-04-09T17:52:51Z

add new entry_points field for the python_distribution() target.
support dependency inference on entry_points.
support pex_binary targets in entry points, which will use the entry_point from the pex_binary() target.

The entry_points field of python_distribution is a dict[str, dict[str, str]] value (just as for setuptools.setup()), where the str value is either on the form of a regular setuptools entry_point or a pants target address. Any value that either starts with a : or that contains a / is considered a target address (for a pex_binary) and resolves to the binary's entry_point.

The reason for not requiring a leading // for target addresses on the entry_point str value, is to support the same syntax 1:1 from the entry_point value on .with_binaries(), to ease migration.

Example BUILD (from this repo)

python_distribution(
  name='pants-packaged',
  ...
  provides=setup_py(
    name='pantsbuild.pants',
    ...
  ),
  entry_points={
    'console_scripts': {
      'pants': 'src/python/pants/bin:pants',
    },
  },
)

Example BUILD, before this change:

... 
  provides=setup_py(
    name='pantsbuild.pants',
    ...
  ).with_binaries(
    pants='src/python/pants/bin:pants',
  )

fixes #11807.
fixes #11808.
closes #11880

Signed-off-by: Andreas Stenius andreas.stenius@svenskaspel.se

[ci skip-rust]

[ci skip-build-wheels]

kaos · 2021-04-09T17:54:44Z

I've left three # type: ignore in there, that I'm not really sure how to handle. That caused these errors:

"Optional[FrozenDict[str, FrozenDict[str, EntryPoint]]]", base class
"NestedDictStringToStringField" defined the type as
"Optional[FrozenDict[str, FrozenDict[str, str]]]")  [assignment]
        value: Optional[FrozenDict[str, FrozenDict[str, EntryPoint]]]
        ^
src/python/pants/backend/python/target_types.py:703:81: error: Incompatible
types in assignment (expression has type
"Optional[FrozenDict[str, FrozenDict[str, EntryPoint]]]", base class
"NestedDictStringToStringField" defined the type as
"Optional[FrozenDict[str, FrozenDict[str, str]]]")  [assignment]
    ...lassVar[Optional[FrozenDict[str, FrozenDict[str, EntryPoint]]]] = None
                                                                         ^
src/python/pants/backend/python/target_types.py:713:5: error: Return type
"Optional[FrozenDict[str, FrozenDict[str, EntryPoint]]]" of "compute_value"
incompatible with return type "Optional[FrozenDict[str, FrozenDict[str, str]]]"
in supertype "NestedDictStringToStringField"  [override]
        def compute_value(
        ^
Found 3 errors in 1 file (checked 455 source files)

Also, I've not done any dependency inference here yet.. either this will be good enough without, and we can address that in #11807 or we resolve both issues in this PR.

src/python/pants/backend/python/goals/setup_py.py

src/python/pants/backend/python/goals/setup_py_test.py

src/python/pants/backend/python/macros/python_artifact.py

jsirois

Thanks @kaos. Sorry for the delay here. I may have missed some context since Eric was working more closely with you on this, so please push back if anything I've pointed out has already been worked out in contradiction.

src/python/pants/backend/python/goals/setup_py.py

src/python/pants/BUILD

src/python/pants/backend/python/goals/setup_py_test.py

Eric-Arellano

Really great stuff! Thank you! If you're willing, I think it would be best to include the dependency inference PR directly in this one. That way, the new deprecated feature is not degrading the experience from before.

You could also maybe leave off the deprecation part of this PR and save for a followup. That way, in the changelog, we would have something like "Add entry_points field to python_distribution" target (new feature), and then the followup could be "Deprecate .with_binaries() in favor of entry_points field".

src/python/pants/BUILD

src/python/pants/backend/python/macros/python_artifact.py

src/python/pants/engine/target.py

src/python/pants/engine/target_test.py

Eric-Arellano · 2021-04-14T04:47:44Z

(Also pardon the delay in reviewing this!)

kaos · 2021-04-14T07:14:56Z

(Also pardon the delay in reviewing this!)

Don't worry about it. We all have priorities to consider :)
I'll look at addressing your and Johns feedback in the coming days. Could be until the weekend before I have time to push anything.

kaos · 2021-07-09T08:15:07Z

OK, this has been long overdue. Sorry for that (I practically had to start over, as I'd forgot all context in this.. so can only guess you have too)
I've cleaned up according to previous review comments. Dropped the deprecation of the .with_binaries() for another PR, and added support for addressing pex_binary targets from python_distribution(entry_points).

I opted to go with a little more liberal approach to determine if it is a build target address. As long as there is a slash in the address, it will never be a setuptools entry point (at the risk of them introducing that.. but I think that risk is rather low)
One can always prefer the absolute address path to be sure (and we can add warnings about it in the future, if need be)
The reason was to stay as close as possible to the supported syntax used for target addresses in the .with_binaries() construct, as can be seen in the diff of the src/python/pants/BUILD file..

It is easy enough to tighten this requirement up, of course (it is just a single if statement doing this check)
https://github.com/pantsbuild/pants/pull/11872/files#diff-4740170d8c26730e2b985731f39b65d20446a0e3ea2def3c82491c3f7c99a17fR158

Dependency inference should work for distribution entry points as well.

Let me know what you think, and I'll fix them up sooner than in another quarter ;)

Eric-Arellano

Thanks @kaos! Pardon the delay, I think I now understand what we're after, specifically with pex_binary and console_scripts. Thanks for the great PR description :)

src/python/pants/backend/python/target_types.py

src/python/pants/backend/python/target_types_rules.py

src/python/pants/backend/python/target_types.py

src/python/pants/backend/python/macros/python_artifact.py

src/python/pants/backend/python/target_types_rules.py

Eric-Arellano · 2021-07-19T22:35:33Z

src/python/pants/backend/python/target_types_test.py

+                    "console_scripts":{
+                        "b_cmd": "project.app:main"
+                    }


You'll also want to check using a pex_binary address, can even be the same one used above for with_binaries.

Also a dep on ansicolors

not sure what you me to test wrgt ansicolors.. ? (how/where?)
I see there is a requirement library for ansicolors in the test, that doesn't seem to do anything.. but don't know how to pull that in, in a useful way.

support dependency inferrence on entry_points. fixes pantsbuild#11807. fixes pantsbuild#11808. Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> [ci skip-rust] [ci skip-build-wheels] support binary targets in entry points.

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

kaos · 2021-07-20T11:48:54Z

src/python/pants/backend/python/target_types_test.py

+    assert_injected(
+        Address("project", target_name="dist-b"),
+        Address("project", relative_file_path="app.py", target_name="my_library"),
+        Address("who_knows", relative_file_path="module.py", target_name="random_lib"),


Notice a small difference here, compared to .with_binaries() here you get a dependency on the owner of the source file that implements the entry point, whereas for .with_binaries() you get a dependency on the pex_binary itself.
Not sure if this is cause for any issues, down the road.. ?

Oh hm....good eye! You're right that this is a problem. When you have an address to a pex_binary, we want a dep on the pex_binary, not the python_library that has the source code for its entry point.

This will require some changes to your dep inference rule. Right now, you are normalizing the whole entry_points field to a set of modules, then doing inference from there. That works when the user specified a module explicitly, but that does not work when they had a target address. Instead, you'll need to differentiate between those two formats when doing inference: if it's a target address, use the same Get(Addresses, UnparsedAddressInputs) used for .with_binaries, else use the Get(PythonModuleOwners, PythonModule) stuff.

you'll need to differentiate between those two formats when doing inference

Maybe update ResolvedPythonDistributionEntryPoints to store two fields: pex_binary_addresses and explicit_modules, then have a property that merges the two into all_entry_points? That allows setup_py.py to use all_entry_points because it doesn't care about the nuance, but dep inference to use the two fields.

Eric-Arellano

Nice, thanks so much for iterating on this Andreas! This is shaping up really nicely.

Eric-Arellano · 2021-07-20T21:03:22Z

src/python/pants/backend/python/goals/setup_py.py

-        if not entry_point:
-            raise InvalidEntryPoint(


FYI to other reviewers: I had identified this code is safe to delete now that we require entry_point for pex_binary, and I asked @kaos to delete it.

src/python/pants/backend/python/goals/setup_py.py

src/python/pants/backend/python/target_types.py

Eric-Arellano · 2021-07-20T21:40:56Z

src/python/pants/backend/python/target_types_rules.py

+                owners.unambiguous
+                or ((maybe_disambiguated,) if maybe_disambiguated else Addresses())
+            )
+            entry_point_addresses += unambiguous_owners  # type: ignore[assignment]


I don't think this will work. Addresses is immutable. Instead, you need to reassign it.

hehe, maybe it is my C background shining through. But this is reassignment. a += b translates to a = a + b. ;)

But lets ensure there is a test, that proves it... ah, well it does verify that code path, when there are more than one entry point in the new entry_points field, so this assert does it: https://github.com/pantsbuild/pants/pull/11872/files#diff-a2ccc3f505ec7e33785464002b3c6192b325d97fcdcda65ad11876f2b5d7a500R448-R451

Hm, to avoid confusion, how about using module_owners = OrderedSet() at the start of this for loop, then calling .update()(?) on it each iteration. After the loop, set entry_point_addresses equal to entry_points.pex_binary_addresses + module_owners.

src/python/pants/backend/python/target_types_rules.py

Eric-Arellano · 2021-07-20T21:49:14Z

src/python/pants/backend/python/target_types_rules.py

+            # ResolvedPexEntryPoint.val is Optional[EntryPoint]
+            if entry_point is None:
+                continue


Interesting, I'm honestly not sure what the right behavior is here. I think probably to error? It doesn't make sense to depend on a pex_binary with entry_point='<none>', as that means there is no entry point. That seems more like an unintentional mistake.

So, perhaps warn? In case it indeed was intentional (even just for testing purposes..)

…up from review feedback # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano

Yay! The functionality and tests look great, now only a couple style things

src/python/pants/backend/python/goals/setup_py.py

src/python/pants/backend/python/target_types.py

Eric-Arellano · 2021-07-21T21:20:14Z

src/python/pants/backend/python/target_types.py

+class PythonDistributionPexableEntryPoint:
+    """Pexable means that the entry point could be from a pex_binary target."""


There's no other type of PythonDistribution, so imo you can remove Pexable.

Suggested change

class PythonDistributionPexableEntryPoint:

"""Pexable means that the entry point could be from a pex_binary target."""

class PythonDistributionEntryPoint:

"""Note that this stores if the entry point comes from an address to a `pex_binary` target."""

If you make this rename, you can rename PythonDistributionEntryPoints (plural) to PythonDistributionEntryPointsField maybe.

Yeah, I avoided PythonDistributionEntryPoint due to there being a plural form, so a mere s to distinguish them by is too little imo. Also, I felt that the name PythonDistributionEntryPointsField got unwieldy long.. and also didn't quite follow the naming pattern for the other fields.. that doesn't have the Field suffix. Maybe we should start using Field suffix for all fields, to be consistent.. ? (it helps making it clearer when the type shows up else where, that it is a field it refers to.. which isn't always obvious, in my eyes at least)

Maybe we should start using Field suffix for all fields, to be consistent.. ?

Probably. We do use Field in a couple names if you ripgrep it, but you're right that not many yet.

But if you prefer that, I'm happy to change it...

src/python/pants/backend/python/target_types_rules.py

Eric-Arellano · 2021-07-21T21:31:30Z

src/python/pants/backend/python/target_types_rules.py

+                owners.unambiguous
+                or ((maybe_disambiguated,) if maybe_disambiguated else Addresses())
+            )
+            entry_point_addresses += unambiguous_owners  # type: ignore[assignment]


Hm, to avoid confusion, how about using module_owners = OrderedSet() at the start of this for loop, then calling .update()(?) on it each iteration. After the loop, set entry_point_addresses equal to entry_points.pex_binary_addresses + module_owners.

src/python/pants/backend/python/target_types_rules.py

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano

Excellent contribution, @kaos 🥳

This is a tricky part of the codebase, and you dove right in :) I'm really excited with how this turned out.

One small followup if you're willing, but I'm going to merge this because that followup doesn't block and this is an excellent change as is.

Eric-Arellano · 2021-07-22T17:11:47Z

src/python/pants/backend/python/target_types_test.py

+                    }
+                },
+            )
+


(In a followup) Could you please add a test for dep inference of entry points defined in the provides=setup_py()?

Sure thing. But trying to remember what's changed since: #11872 (comment)

[...] you can still use the old setup_py() way of doing it, only you'll miss out a) dependency inference

Hmm... looking at the inject_python_distribution_dependencies I don't think that we infer dependencies on entry_points from setup_py() at all.. so that test should not work ;)

But then again, that was the point of #11807 which supposedly was incorporated into this PR... eeh.. a little too much water under the bridge from start to finnish, I'd say..

So, question becomes, do we want to spend time fixing that dep inference, or simply leave be waiting for #12410

Ah, okay. Makes sense.

Totally depends on what you have the time and interest to do: #12410 is more of a blocker b/c right now we have two Pants-provided ways of doing something, and we want to get rid of the .with_binaries() tech debt.

It would also be great to add dep inference for entry points defined inside a setup_py(), but that isn't a blocker. It's a new feature. If you have the time and interest, that'd be great - okay if not. Either way, maybe create a issue to track this so we don't forget it's a feature request?

I'll post a PR deprecating .with_binaries() tomorrow.
Can also look into the dep inference on setup_py entry_points, shouldn't be too difficult, now :)

Deprecation PR: #12413

Dep inference for setup_py() entry points PR: #12414

kaos commented Apr 9, 2021

View reviewed changes

src/python/pants/backend/python/goals/setup_py.py Outdated Show resolved Hide resolved

kaos commented Apr 9, 2021

View reviewed changes

src/python/pants/backend/python/goals/setup_py_test.py Show resolved Hide resolved

kaos commented Apr 9, 2021

View reviewed changes

src/python/pants/backend/python/macros/python_artifact.py Outdated Show resolved Hide resolved

kaos mentioned this pull request Apr 10, 2021

support dependency inferrence on distribution entry_points #11880

Closed

benjyw requested review from jsirois and Eric-Arellano April 12, 2021 16:01

jsirois reviewed Apr 13, 2021

View reviewed changes

Eric-Arellano reviewed Apr 14, 2021

View reviewed changes

kaos changed the title ~~deprecate .with_binaries() in favor of a target field on python_distr…~~ add entry_points field on python_distribution Jul 8, 2021

kaos force-pushed the kaos/with_binaries branch from 5cfacde to 056bc5e Compare July 9, 2021 07:59

kaos requested review from Eric-Arellano and jsirois July 9, 2021 08:06

kaos force-pushed the kaos/with_binaries branch from 056bc5e to 8e1f580 Compare July 9, 2021 08:29

Eric-Arellano reviewed Jul 19, 2021

View reviewed changes

kaos added 3 commits July 20, 2021 08:58

test_inject_python_distribution_dependencies: fix test issue.

cb9f0a7

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

incorporate review feedback.

9f979b5

kaos force-pushed the kaos/with_binaries branch from fcb3c3a to 9f979b5 Compare July 20, 2021 11:44

kaos commented Jul 20, 2021

View reviewed changes

kaos requested a review from Eric-Arellano July 20, 2021 11:49

Eric-Arellano reviewed Jul 20, 2021

View reviewed changes

fix dep inference issue on pex binary entry points. refactoring/clean…

971c387

…up from review feedback # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano approved these changes Jul 21, 2021

View reviewed changes

Eric-Arellano requested a review from stuhood July 21, 2021 21:32

stuhood approved these changes Jul 21, 2021

View reviewed changes

Update src/python/pants/backend/python/goals/setup_py.py

3cb6406

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

kaos and others added 4 commits July 22, 2021 08:59

Update src/python/pants/backend/python/goals/setup_py.py

f717a60

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

Update src/python/pants/backend/python/goals/setup_py.py

e59def5

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

review feedback cleanups.

af0d30c

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

ensure consistent use of entry_points "category" rather than "section".

7e9e27e

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano approved these changes Jul 22, 2021

View reviewed changes

Eric-Arellano merged commit ffee645 into pantsbuild:main Jul 22, 2021

kaos deleted the kaos/with_binaries branch July 22, 2021 17:50

		class PythonDistributionPexableEntryPoint:
		"""Pexable means that the entry point could be from a pex_binary target."""

add entry_points field on python_distribution #11872

add entry_points field on python_distribution #11872

Conversation

kaos commented Apr 9, 2021 • edited Loading

kaos commented Apr 9, 2021

jsirois left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano commented Apr 14, 2021

kaos commented Apr 14, 2021

kaos commented Jul 9, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos commented Apr 9, 2021 •

edited

Loading

kaos Jul 21, 2021 •

edited

Loading