Support pex_binary addresses in setup_py(entry_points) along with dep… #12414

kaos · 2021-07-23T08:06:13Z

With the new entry_points field on python_distribution that supports arbitrary entry points for the setuptools.setup() call, including getting the entry point method from a pex_binary targets entry point and dependency inference, this PR brings the same features to the setup_py(entry_points) object field.

By uniting all entry point processing to the resolve_python_distribution_entry_points and inject_python_distribution_dependencies rules, it is easier to treat all the different sources of entry points the same.

Legacy with_binaries() processing in the generate_chroot rule can thus be removed in favor of the updated python distribution rules mentioned above.

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]

…endency inference. Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # 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-23T08:09:20Z

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

@@ -384,7 +384,7 @@ def test_generate_chroot_entry_points(chroot_rule_runner: RuleRunner) -> None:
                    name='foo', version='1.2.3',
                    entry_points={
                        "console_scripts":{
-                            "foo_qux":"foo.baz.qux",
+                            "foo_qux":"foo.baz.qux:main",


Good to see that we catch bugs in the test suite :P

So, before it was inferring a dep on the python_library and now it does it on the pex_binary? Cool!

Well, there are too many foo, bar, baz and qux in this test. But no, I don't think so. It refers to a module (that is not part of any target, so "orphan") but being a console script, it must point at a method, not just a module.

Well, there are too many foo, bar, baz and qux in this test

Yep, agreed. I personally don't like foo and friends. https://www.techyourchance.com/never-use-foo-bar-baz/ Feel free to update in another change if you'd like :)

kaos · 2021-07-23T08:10:36Z

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

+    """
+
+    entry_points_field: Optional[PythonDistributionEntryPointsField] = None
+    provides_field: Optional[PythonProvidesField] = None


I extended the support for fields to look at for resolving entry points, so we get the same logic applied consistently across all different ways to provide entry points.

To be extra defensive, you could add a def __post__init() that raises AssertionError if both are unspecified.

kaos · 2021-07-23T08:12:12Z

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

        return ResolvedPythonDistributionEntryPoints()

-    address = request.entry_points_field.address
-    classified_entry_points = list(_classify_entry_points(field_value))
+    classified_entry_points = list(_classify_entry_points(all_entry_points))


Had a bit of a struggle with MyPy here (not yet that experienced with python type hints..)
So ended up resolving to using a few intermediary variables to overcome the Optionals. If there is a better way....

One way to do this is to set all_entry_points: MyType[Foo, Bar] at the top of this whole conditional block. Note that you don't set the actual value to anything. It's only to let MyPy know "hey I'm gonna have this variable with this type".

Will try that next time. Not feeling like digging around any more in this as it is working, unless it's too painful for you eyes ;)

kaos · 2021-07-23T08:14:23Z

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

@@ -209,7 +239,7 @@ async def resolve_python_distribution_entry_points(
    # Check that we only have targets with a pex entry_point field.
    for target in targets:
        if not target.has_field(PexEntryPointField):
-            raise InvalidFieldException(
+            raise InvalidEntryPoint(


This got my head scratching. I'd like to be consistent, but some code raise InvalidFieldException, where as other InvalidEntryPoint... feels like InvalidEntryPoint ought to inherit from InvalidFieldException too, at least... so a catch for field exceptions would catch this.

Any way, I went with keeping the tests as is, as that is the user facing interface. But I think we could make a do-over here to make it more consistent. ;)

That sounds good to have InvalidEntryPoint subclass InvalidFieldException, definitely. And makes sense if you want to do in a followup to keep this PR better scoped.

Issue #12430

kaos · 2021-07-23T08:15:31Z

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

    return InjectedDependencies(
-        Addresses(module_owners) + with_binaries_addresses + all_entry_points.pex_binary_addresses


with binaries moved to the resolve entry points rule.

kaos · 2021-07-23T08:16:01Z

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

+            Address("project", relative_file_path="app.py", target_name="my_library"),
+        ],
+    )
+


test case driving the change.

Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # 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-23T09:42:29Z

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

            ),
-        )


got to clean up a lot of code, as that functionality moved to resolve entry points rule.

kaos · 2021-07-23T09:45:16Z

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

        f"{exported_addr}'s field `entry_points`": entry_points_from_field,
-        f"{exported_addr}'s field `provides=setup_py(..., entry_points={...})`": entry_points_from_provides,


The with_binaries() are now merged during resolve of the provides field, so we're left with the two resolves for each field, entry points and provides, respectively. The downside is, that when there is a conflict with a entry point from the distributions field, we can't say if it is with setup_py().with_binaries or with setup_py(entry_points) any more..

That is totally reasonable, esp. becauase with_binaries() is now deprecated. Thanks for the disclaimer!

kaos · 2021-07-23T09:46:51Z

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

@@ -1011,7 +982,7 @@ def has_args(call_node: ast.Call, required_arg_ids: Tuple[str, ...]) -> bool:

 def merge_entry_points(
    *all_entry_points_with_descriptions_of_source: Tuple[str, Dict[str, Dict[str, str]]]
-) -> Dict[str, List[str]]:


Changed signature, to stay true to our "normalized" structure for entry points. The List[str] form for entry points is applied at the last moment when outputting for the setup kwargs.

Good change!

kaos · 2021-07-23T09:47:21Z

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

@@ -1024,22 +995,22 @@ def merge_entry_points(
            for ep_name, entry_point in entry_points.items():
                merged[category][ep_name].append((description_of_source, entry_point))

-    def _merge_entry_point(


adjusted name to be true to what it does

kaos · 2021-07-23T09:48:42Z

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

+
+        with_binaries = request.provides_field.value.binaries
+        if with_binaries:
+            all_entry_points = merge_entry_points(


reusing merge_entry_points ;)

kaos · 2021-07-23T09:51:13Z

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

@@ -384,7 +384,8 @@ def test_generate_chroot_entry_points(chroot_rule_runner: RuleRunner) -> None:
                    name='foo', version='1.2.3',
                    entry_points={
                        "console_scripts":{
-                            "foo_qux":"foo.baz.qux",
+                            "foo_qux":"foo.baz.qux:main",
+                            "foo_bin":":foo-bin",


second commit was to pick this up, which it wasn't before..
that is, target address to pex_binary in entry points of setup_py..

Eric-Arellano

This is really great! Thanks @kaos, excellent improvements 👏

Outside of a couple small style things, my main feedback is that it'd be helpful to add a PR description. Doesn't need to be long, but explaining a little what this change is about and giving a brief overview of how you achieve it.

Eric-Arellano · 2021-07-27T02:31:50Z

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

        f"{exported_addr}'s field `entry_points`": entry_points_from_field,
-        f"{exported_addr}'s field `provides=setup_py(..., entry_points={...})`": entry_points_from_provides,


That is totally reasonable, esp. becauase with_binaries() is now deprecated. Thanks for the disclaimer!

Eric-Arellano · 2021-07-27T02:34:17Z

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

+    entry_points_from_field = {
+        category: {ep_name: ep_val.entry_point.spec for ep_name, ep_val in entry_points.items()}
+        for category, entry_points in entry_points_field.val.items()
+    }

-    # Collect any entry points from the setup_py() object. Note that this was already normalized in
-    # `python_artifacts.py`.
-    entry_points_from_provides = setup_kwargs.get("entry_points", {})
+    entry_points_from_provides = {
+        category: {ep_name: ep_val.entry_point.spec for ep_name, ep_val in entry_points.items()}
+        for category, entry_points in provides_field.val.items()
+    }


Normally, we prefer comprehensions like this. But here, it looks like you're iterating over entry_points.items() more than necessary, and so the style guide says:

If constructing multiple collections by iterating over the same original collection, use a for loop for performance.

Here, I think you would have for ep_name, ep_val in entry_points.items() in your outer loop. Then two inner loops over provides_field.val.items() and entry_points_field.val.items().

Eh.. it is not the same entry_points.items(), the first comes from entry_points_field.val, the other from provides_field.val. ;)

Could however have a little helper function to reduce code duplication, and this would become super clear :)

Eric-Arellano · 2021-07-27T02:35:21Z

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

@@ -1011,7 +982,7 @@ def has_args(call_node: ast.Call, required_arg_ids: Tuple[str, ...]) -> bool:

 def merge_entry_points(
    *all_entry_points_with_descriptions_of_source: Tuple[str, Dict[str, Dict[str, str]]]
-) -> Dict[str, List[str]]:


Good change!

Eric-Arellano · 2021-07-27T02:36:06Z

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

-        for _, entry_point in entry_points_with_source:
-            return f"{name}={entry_point}"
+        _, entry_point = entry_points_with_source[0]
+        return name, entry_point


Excellent change! This is much more obvious what it does. Nice :D

This is the most rewarding part to me, tbh. Learning to write comprehensible code. Normally, it's just I that have to grok my code, and my peers are lost in the mist of what it is supposed to do in the first place, so there is no real pressure to be super clear with the intentions. But, it will be a lot easier to maintain in the long run, so well worth spending a few minutes to get there. So, that's what I'm happy about, to be able to get there with more ease than before.. ;)

Eric-Arellano · 2021-07-27T02:37:02Z

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

@@ -384,7 +384,7 @@ def test_generate_chroot_entry_points(chroot_rule_runner: RuleRunner) -> None:
                    name='foo', version='1.2.3',
                    entry_points={
                        "console_scripts":{
-                            "foo_qux":"foo.baz.qux",
+                            "foo_qux":"foo.baz.qux:main",


So, before it was inferring a dep on the python_library and now it does it on the pex_binary? Cool!

Eric-Arellano · 2021-07-27T02:38:49Z

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

+    """
+
+    entry_points_field: Optional[PythonDistributionEntryPointsField] = None
+    provides_field: Optional[PythonProvidesField] = None


To be extra defensive, you could add a def __post__init() that raises AssertionError if both are unspecified.

Eric-Arellano · 2021-07-27T02:40:23Z

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

@@ -209,7 +239,7 @@ async def resolve_python_distribution_entry_points(
    # Check that we only have targets with a pex entry_point field.
    for target in targets:
        if not target.has_field(PexEntryPointField):
-            raise InvalidFieldException(
+            raise InvalidEntryPoint(


That sounds good to have InvalidEntryPoint subclass InvalidFieldException, definitely. And makes sense if you want to do in a followup to keep this PR better scoped.

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

Eric-Arellano · 2021-07-27T02:46:11Z

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

        return ResolvedPythonDistributionEntryPoints()

-    address = request.entry_points_field.address
-    classified_entry_points = list(_classify_entry_points(field_value))
+    classified_entry_points = list(_classify_entry_points(all_entry_points))


One way to do this is to set all_entry_points: MyType[Foo, Bar] at the top of this whole conditional block. Note that you don't set the actual value to anything. It's only to let MyPy know "hey I'm gonna have this variable with this type".

Eric-Arellano · 2021-07-27T02:46:55Z

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

+        for category, entry_points in chain(
+            distribution_entry_points.explicit_modules.items(),
+            provides_entry_points.explicit_modules.items(),
+        )


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

Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # 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]

…ts_dep_inference

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

# 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

🤘

…19149) Fixes #12430 #12414 (comment) > This got my head scratching. I'd like to be consistent, but some code > raise InvalidFieldException, where as other InvalidEntryPoint... feels > like InvalidEntryPoint ought to inherit from InvalidFieldException too, > at least... so a catch for field exceptions would catch this. > > Any way, I went with keeping the tests as is, as that is the user > facing interface. But I think we could make a do-over here to make it > more consistent. ;)

kaos mentioned this pull request Jul 23, 2021

add entry_points field on python_distribution #11872

Merged

kaos requested a review from Eric-Arellano July 23, 2021 08:07

kaos commented Jul 23, 2021

View reviewed changes

Eric-Arellano approved these changes Jul 27, 2021

View reviewed changes

kaos mentioned this pull request Jul 27, 2021

The InvalidEntryPoint should subclass InvalidFieldException #12430

Closed

kaos and others added 5 commits July 27, 2021 10:04

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

fc73526

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

apply review feedback.

262b4e6

Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # 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]

Merge remote-tracking branch 'upstream/main' into setup_py_entry_poin…

43197b1

…ts_dep_inference

cleanup .with_binaries() deprecation message.

85b02c7

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

rename test

0bcccc7

# 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 27, 2021

View reviewed changes

Eric-Arellano merged commit 54fb09d into pantsbuild:main Jul 27, 2021

kaos deleted the setup_py_entry_points_dep_inference branch July 27, 2021 20:54

kaos mentioned this pull request May 25, 2023

Have package_dists.InvalidEntryPoint subclass InvalidFieldException #19149

Merged

		return InjectedDependencies(
		Addresses(module_owners) + with_binaries_addresses + all_entry_points.pex_binary_addresses

		f"{exported_addr}'s field `entry_points`": entry_points_from_field,
		f"{exported_addr}'s field `provides=setup_py(..., entry_points={...})`": entry_points_from_provides,

Support pex_binary addresses in setup_py(entry_points) along with dep… #12414

Support pex_binary addresses in setup_py(entry_points) along with dep… #12414

Conversation

kaos commented Jul 23, 2021 • edited Loading

Rust tests and lints will be skipped. Delete if not intended.

Building wheels and fs_util will be skipped. Delete if not intended.

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos Jul 23, 2021 • edited Loading

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

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

kaos commented Jul 23, 2021 •

edited

Loading

kaos Jul 23, 2021 •

edited

Loading