Add `python_resolve` field to `protobuf_source` and `thrift_source` to support multiple resolves with codegen #14693

Eric-Arellano · 2022-03-03T21:28:46Z

Problem

Generated code needs associated runtime libraries, e.g. protobuf. Those must come from a python_requirement which by definition have a single resolve.

Before this PR, it was impossible to have >1 python_requirement target used for each runtime library and every protobuf_source target would depend on the same python_requirement. Practically, this means codegen could only work with code belonging to a single resolve.

Solution

Add a python_resolve field to codegen targets so that they can indicate which python_requirement they should be using. Our new dependency inference from #14691 wires this up properly.

If you want the same generated code to work with multiple resolves, you will need to use parametrize. That gets tricky when you consider a codegen target generating for multiple languages, as explained at the bottom of #14484, but there is a decent workaround via configurations. That will be tackled in a followup.

FYI: lazy validation of runtime library

#14691 asserts that there is exactly 1 runtime library in the project; now we assert that for each resolve.

Key nuance: this check is lazy. If you have 2 resolves, but your codegen targets only use 1 of the 2, then we will never end up checking that there is a runtime library defined for the 2nd resolve. That is important so that, for example, if you have a pants-plugins resolve we don't force you to add protobuf unnecessarily to it.

[ci skip-rust]
[ci skip-build-wheels]

…o support multiple resolves with codegen # 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

Note that this is on 2.10 branch

Thanks to @tdyas and @stuhood for helping to figure out this very tricky problem!

Eric-Arellano · 2022-03-03T21:30:11Z

src/python/pants/backend/codegen/utils.py

    recommended_requirement_name: str,
    recommended_requirement_url: str,
    disable_inference_option: str,
 ) -> Address:
    addresses = [
        module_provider.addr
        for module_provider in module_mapping.providers_for_module(
-            runtime_library_module, resolve=None
+            runtime_library_module, resolve=resolve


Reminder that if --no-enable-resolves, Pants will have set the resolve name to <ignore-me> or something like that. So everything ends up being in the same resolve.

Eric-Arellano · 2022-03-03T21:31:14Z

src/python/pants/backend/codegen/utils.py

+            f"Alternatively, change the resolve field for {codegen_address} to use a different "
+            "resolve from `[python].resolves`."
+        )
+        if resolves_enabled


If resolves are enabled, then you must not have two python_requirement targets for the same project in the resolve. That is nonsensical.

tdyas

lgtm

…o support multiple resolves with codegen (pantsbuild#14693) Closes pantsbuild#14484. ## Problem Generated code needs associated runtime libraries, e.g. `protobuf`. Those must come from a `python_requirement` which by definition have a single `resolve`. Before this PR, it was impossible to have >1 `python_requirement` target used for each runtime library and every `protobuf_source` target would depend on the same `python_requirement`. Practically, this means codegen could only work with code belonging to a single resolve. ## Solution Add a `python_resolve` field to codegen targets so that they can indicate which `python_requirement` they should be using. Our new dependency inference from pantsbuild#14691 wires this up properly. If you want the same generated code to work with multiple resolves, you will need to use `parametrize`. That gets tricky when you consider a codegen target generating for multiple languages, as explained at the bottom of pantsbuild#14484, but there is a decent workaround via configurations. That will be tackled in a followup. ### FYI: lazy validation of runtime library pantsbuild#14691 asserts that there is exactly 1 runtime library in the project; now we assert that for each resolve. Key nuance: this check is lazy. If you have 2 resolves, but your codegen targets only use 1 of the 2, then we will never end up checking that there is a runtime library defined for the 2nd resolve. That is important so that, for example, if you have a `pants-plugins` resolve we don't force you to add `protobuf` unnecessarily to it. [ci skip-rust] [ci skip-build-wheels]

…o support multiple resolves with codegen (Cherry-pick of #14693) (#14698) Closes #14484. ## Problem Generated code needs associated runtime libraries, e.g. `protobuf`. Those must come from a `python_requirement` which by definition have a single `resolve`. Before this PR, it was impossible to have >1 `python_requirement` target used for each runtime library and every `protobuf_source` target would depend on the same `python_requirement`. Practically, this means codegen could only work with code belonging to a single resolve. ## Solution Add a `python_resolve` field to codegen targets so that they can indicate which `python_requirement` they should be using. Our new dependency inference from #14691 wires this up properly. If you want the same generated code to work with multiple resolves, you will need to use `parametrize`. That gets tricky when you consider a codegen target generating for multiple languages, as explained at the bottom of #14484, but there is a decent workaround via configurations. That will be tackled in a followup. ### FYI: lazy validation of runtime library #14691 asserts that there is exactly 1 runtime library in the project; now we assert that for each resolve. Key nuance: this check is lazy. If you have 2 resolves, but your codegen targets only use 1 of the 2, then we will never end up checking that there is a runtime library defined for the 2nd resolve. That is important so that, for example, if you have a `pants-plugins` resolve we don't force you to add `protobuf` unnecessarily to it. [ci skip-rust] [ci skip-build-wheels]

…y targets (#14486) If you have >1 target for the same first-party module, that is now okay with dependency inference as long as they each have a distinct resolve. This is key for `parametrize()` to work properly: ```python python_source( name="foo", source="foo.py", resolve=parametrize("a", "b"), ) ``` Note how codegen targets should have a `python_resolve` field now, per #14693. That allows support for multiple runtime libraries like `thrift` and `protobuf` dependencies. Now Python will only infer deps on codegen targets using the correct resolve for its runtime library. [ci skip-rust] [ci skip-build-wheels]

…y targets (pantsbuild#14486) If you have >1 target for the same first-party module, that is now okay with dependency inference as long as they each have a distinct resolve. This is key for `parametrize()` to work properly: ```python python_source( name="foo", source="foo.py", resolve=parametrize("a", "b"), ) ``` Note how codegen targets should have a `python_resolve` field now, per pantsbuild#14693. That allows support for multiple runtime libraries like `thrift` and `protobuf` dependencies. Now Python will only infer deps on codegen targets using the correct resolve for its runtime library. [ci skip-rust] [ci skip-build-wheels]

…y targets (Cherry-pick of #14486) (#14765) If you have >1 target for the same first-party module, that is now okay with dependency inference as long as they each have a distinct resolve. This is key for `parametrize()` to work properly: ```python python_source( name="foo", source="foo.py", resolve=parametrize("a", "b"), ) ``` Note how codegen targets should have a `python_resolve` field now, per #14693. That allows support for multiple runtime libraries like `thrift` and `protobuf` dependencies. Now Python will only infer deps on codegen targets using the correct resolve for its runtime library. [ci skip-rust] [ci skip-build-wheels]

Add python_resolve field to protobuf_source and thrift_source t…

033dd93

…o support multiple resolves with codegen # 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 requested review from stuhood and tdyas March 3, 2022 21:28

Eric-Arellano commented Mar 3, 2022

View reviewed changes

tdyas approved these changes Mar 3, 2022

View reviewed changes

Eric-Arellano merged commit fe41887 into pantsbuild:2.10.x Mar 3, 2022

Eric-Arellano deleted the codegen-py-resolve-10 branch March 3, 2022 22:46

This was referenced Mar 4, 2022

Python dependency inference supports multiple resolves for first-party targets #14486

Merged

generate and compile Go from Protobuf #14714

Merged

Eric-Arellano mentioned this pull request Mar 11, 2022

Python dependency inference supports multiple resolves for first-party targets (Cherry-pick of #14486) #14765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `python_resolve` field to `protobuf_source` and `thrift_source` to support multiple resolves with codegen #14693

Add `python_resolve` field to `protobuf_source` and `thrift_source` to support multiple resolves with codegen #14693

Eric-Arellano commented Mar 3, 2022

Eric-Arellano left a comment

Eric-Arellano Mar 3, 2022

Eric-Arellano Mar 3, 2022

tdyas left a comment

Add python_resolve field to protobuf_source and thrift_source to support multiple resolves with codegen #14693

Add python_resolve field to protobuf_source and thrift_source to support multiple resolves with codegen #14693

Conversation

Eric-Arellano commented Mar 3, 2022

Problem

Solution

FYI: lazy validation of runtime library

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Mar 3, 2022

Choose a reason for hiding this comment

Eric-Arellano Mar 3, 2022

Choose a reason for hiding this comment

tdyas left a comment

Choose a reason for hiding this comment

Add `python_resolve` field to `protobuf_source` and `thrift_source` to support multiple resolves with codegen #14693

Add `python_resolve` field to `protobuf_source` and `thrift_source` to support multiple resolves with codegen #14693