Protobuf sources does not work with parametrized resolves

[omri-cavnue]

Describe the bug
When introducing multiple resolves into the repo, all of the python sources and test targets seem to work. However, the protobuf_sources does not work against multiple resolves. On a single resolve, the target works. But when parametrizing against multiple resolves, it complains about proto files not being found when they are imported into other proto files.

Pants version
2.21.0

OS
Both mac and linux

Additional info
The repo structure is similart to:

   ├── proto
    | └── messages
    │    └── example
    │       └── v1
    │           └── person.proto
         └── example_2
            └── v1
    │           └── dog.proto
    | └── BUILD
   ├── python
    | └── packages
    │    └── messages
    |         └── src

Here, the dog.proto would import person.proto to identify an owner.

syntax = "proto3";
package messages.example_v2.v1;
import "messages/example/v1/owner.proto";

message Dog {
   string name = 1;
   example.v1.Person owner = 2;
}

and it complains about messages/example/v1/person.proto File not found.
The BUILD file is as follows:

protobuf_sources(
    name = "protos",
    python_source_root = "python/packages/messages/src",
    sources = ["messages/**/*.proto"],
    python_resolve = parametrize("core", "dev"),
)

With one resolve, this works as expected. With multiple resolves, it complains that the person.proto file is not found when trying to create python sources for the dog proto

[benjyw]

Hi, it would be really helpful if you could create a repo containing a minimal repro of this, with a README explaining how to reproduce the bug. Then all we need to do is clone your repo and run that, and we can be sure we're all looking at the same thing. Thanks!

[huonw]

I recall a steady series of fixes to parametrisation in 2.22.0 and 2.23 (currently in pre-release), so potentially you could try upgrading to one of those and see if the problem is already resolved.

If not, a standalone reproducer as Benjy describes would be great, thank you!

[omri-cavnue]

Hi all, I've tried with 2.22.0 and still see the issue. I have made a sample repo here where the README explains the problem and steps to reproduce

[huonw]

Thanks. I can reproduce the issue with 2.23.0a0 too. I don't have input on resolving it though, but maybe someone else does.

(BTW, not related to that issue, just picking up from the description: Pants will manage the codegen and venvs internally, so pants export ... and pants export-codegen ... don't generally need to be run, except for things like IDE support (https://www.pantsbuild.org/stable/docs/using-pants/setting-up-an-ide). Running pants test :: alone does the right thing, including picking up the latest codegen and/or venv changes 😄 )

[omri-cavnue]

@huonw thanks for taking a look. I added in the export commands to make it most similar to our actual IDE and repo setup which is a bit more complex, but good to know that it's not needed for this repro.

@benjyw do you have any ideas on resolving the issue?

[benjyw]

Thanks for the robust repro, which reproduces the issue for me. I will take a look ASAP.

[benjyw]

These warnings seem pertinent:

13:42:55.22 [WARN] The target messages/proto/definitions/messages/animal/v1/dog.proto:../../../../protos@python_resolve=prod imports `definitions/third_party/bq/v1/bq_field.proto` in the file messages/proto/definitions/messages/animal/v1/dog.proto, but Pants cannot safely infer a dependency because more than one target owns this file, so it is ambiguous which to use: ['messages/proto/definitions/third_party/bq/v1/bq_field.proto:../../../../protos@python_resolve=dev', 'messages/proto/definitions/third_party/bq/v1/bq_field.proto:../../../../protos@python_resolve=prod'].

[benjyw]

The parametrization introduces two sources of each .proto to .proto dependency. And because dep inference isn't disambiguating via the specific parametrization, Pants selects neither dep, and you end up with those errors.

I'll see how this is handled in other parametrized contexts, such as dep inference between .py files.

[benjyw]

Interestingly:

$ pants dependencies python/services/dog-walker/src/dog_walker/service.py 
messages/proto/definitions/messages/animal/v1/dog.proto:../../../../protos@python_resolve=prod
messages/proto/definitions/messages/human/v1/person.proto:../../../../protos@python_resolve=prod

So the dependencies goal at least is able to do the right thing.

[benjyw]

So for Python dep resolution the resolve is used in a deep way to disambiguate:

pants/src/python/pants/backend/python/dependency_inference/module_mapper.py

Line 187 in 86d8729

If `resolve` is None, will not consider resolves, i.e. any `python_source` et al can be

The same is not true for proto dep resolution.

The problem is, I'm not sure how it could be. The resolve is a python-specific concept that is passed through the protobuf_sources as a "plugin field", but isn't acted on by it. The parametrize "duplicates" the protobuf_sources target but along an axis that is not known to the dep inference code. Hmmm.

[benjyw]

@omri-cavnue have you found a workaround?

[omri-cavnue]

@benjyw no not yet. I've tried a few solutions, including adding the files to the dependencies list manually, as well as creating a matrix via parametrization in the dependencies field, but neither works and these are also very manual solutions

[omri-cavnue]

@benjyw I wanted to follow up here to see whether there is anything in the pipeline to unblock this issue?

[benjyw]

I haven't looked at it in depth yet, but thanks for the reminder. Will try and get to it ASAP.

[tdyas]

I believe I am hitting this issue with trying to support a Pants plugin with multiple resolves where each resolve has a different Pants version (since I want to support the plugin across Pants releases). As soon as I have two resolves and both contain the pantsbuild.pants package, Pants cannot infer dependencies any more for that package. I'll try to solve this week since I now have a personal interest to do so.

EDIT: Spoke too soon, it was because of not having the correct module_mapping on my python_requirements targets. That said, this issue does seem like it involves Pants not tracking the resolve correctly for these targets (even if it is doing so correctly for regular python_source targets).

[benjyw]

@omri-cavnue , looking at this more closely - now that I think of it, why are you parametrizing the proto targets at all? Is this so that the runtime proto library comes from the appropriate resolve in each case?

[omri-cavnue]

@benjyw That's right. We have our protos as first party dependencies and would like to use them across resolves, so we are parametrizing them so that we can use compiled python definitions across resolves

[benjyw]

And if you don't parameterize the proto targets, Pants assigns the generated python code to the default resolve or something?

[tdyas]

What is interesting here is that the protobuf Python rules do deal with the python_resolve field:

pants/src/python/pants/backend/codegen/protobuf/python/python_protobuf_module_mapper.py

Line 61 in 3c15034

resolve = tgt[PythonResolveField].normalized_value(python_setup)

[benjyw]

Yes, but proto dep inference is entirely in the proto rules, which don't know about this "plugin field".

[tdyas]

So we need a way for protobuf dep inference to account for resolve-like plugin fields then?

[tdyas]

Thought: Add a mix-in ResolveLike that all plugin fields that are resolve fields should mix in. Protobuf dependency inference can then partition the universe of protobuf targets by the values of any such fields. Then run dependency inference within each partition.

[tdyas]

Very rough draft of the partitioning solution: #21918

It does not work currently due to rule graph errors, but you can see the potential solution direction.

[tdyas]

After some more work on the draft PR, it fixes the bug in the reproduction repository. Although it still needs a unit test and there is a limitation if more than 1 resolve-like field were registered on a protobuf target.

[omri-cavnue]

And if you don't parameterize the proto targets, Pants assigns the generated python code to the default resolve or something?

@benjyw yup exactly, it will only work with the default resolve in that case