https://pantsbuild.org/ logo
Join Slack
Powered by
<@U04468GC7ED> it looks like this is the third use...
# development
l
@gorgeous-winter-99296 it looks like this is the third user who has run into this problem (including yourself). Does it make sense for us to give it a crack? I'm happy to take a look, since it involves a bunch of stuff I don't know much about.
g
It's a bit out of scope for me in the coming weeks, at least. I've not bothered looking too deep yet because we have a handful of proto files so it's not too painful to work around.
I think in general there are lots of sharp edges when you start using multiple resolves; which unfortunately some domains force you to do. So I'm not sure if tackling individual places is the right place to start.
l
oh, so like maybe there is some more fundamental solution that would make this kind of problem go away
Do you know of a more painful but plausibly solvable multi-resolve issue?
g
The one I find is most painful is if you start using 
parametrize
on sources one ends up having to do 
@resolve=...
on 
run
and 
test
. Then suddenly all your targets exist in triplicate across the whole codebase, at a minimum. We have four resolves at this point (three just to fix Torch's broken wheels), and by extension every 
python_source
in shared library has 
@resolve={cpu,gpu,base,...}
. I'm not sure if this is really a configuration issue on our end, but I think to resolve the ambiguity of which torch we want we end up with this. I think that is the same issue that also existed in the issue you linked, etc.
l
I've heard at least someone else remark about wanting to have a kind of 'default' resolve when a resolve-parametrized target is specified without qualifying fully with the resolve.
c
Oh, that’s likely due to the field being a “core_field” rather than “moved”, as indicated by the error message. The field comes from here https://github.com/pantsbuild/pants/blob/78111bbc105418dc2f419181e67c8ec2f80f439a/[…]thon/pants/backend/codegen/protobuf/python/additional_fields.py so it’s a plugin field, meaning there’s lacking support for registering a plugin field as being “moved” for a target generator. See the 
Target
class for how plugin fields are registered: https://github.com/pantsbuild/pants/blob/78111bbc105418dc2f419181e67c8ec2f80f439a/[…]thon/pants/backend/codegen/protobuf/python/additional_fields.py More core vs moved fields on the TargetGenerator class: https://github.com/pantsbuild/pants/blob/78111bbc105418dc2f419181e67c8ec2f80f439a/src/python/pants/engine/target.py#L1009-L1038
l
Interesting, would this issue be a good context for tackling that more generic problem?
c
yes, I’d say that’s about the ~only way to solve it properly.
l
I am taking a crack at this. Will be nice to learn more about how targets work.
💪 1
Just hacking at it unscrupulously, doing:
Copy code
ProtobufSourcesGeneratorTarget.moved_fields = (*ProtobufSourcesGeneratorTarget.moved_fields, ProtobufPythonResolveField)
is enough to get it to work in my test situation. I'll make a (probably gruesome) PR with a small surface area that fixes it for this one case, and as a place to have the more general conversation about how to deal with parametrizable plugin fields in target generators.
@curved-television-6568 I started a PR with a hacked thing indicating the nature of the problem and I have a comment about ways to solve it. https://github.com/pantsbuild/pants/pull/20059#issuecomment-1771572260 Are you able to weigh in on whether it is a sensible direction?
👀 1
👍 1
Thank you for all the feedback so far, in the meanwhile I managed to get something that's actually green-looking in the PR. I marked it as ready for review on those grounds, but I expect that several of the things in there could use further adjustment. Noted some in this comment. I got nervous about auto-moving all plugin fields registered to generators, and didn't do it yet.
Need to also do an e2e type manual check that it does fix the original problem with proto/python/parametrized-resolves
👍 1
We are all good, added description and e2e testing result to the PR: https://github.com/pantsbuild/pants/pull/20059
c
great work. let’s attract a few more eyes on this before we land it 😉 https://github.com/pantsbuild/pants/pull/20059
🙌 1
l
Replied to the comments and fixed up the situation with the tests. As to separating the change for proto into a separate follow-on PR let me know. I am not sure which way I lean about splitting it out, so happy either way.
c
The reason to split this into two PRs is to get a proper note about it in the release for the new feature (register moved plugin field), while the bugfix goes into another section fixing the issue using this feature, so in that regard this PR does two things unless we split it.. 🙂
in other words, it’s mostly for changelog reasons..
l
👍 👍 will split it out
does it mean though that, since the 'feature' is targeted to main, that the 'fix' part will not be patched onto earlier minor versions?
c
awesome, thanks. If we want to backport the fix, we’d also backport the feature first, which we can do when deemed low-risk (which I think this is)
👍 1
l
Moved the fix out into https://github.com/pantsbuild/pants/pull/20069 and relabeled as new feature.
2 Views
Open in Slack
PreviousNext
Powered by