Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling both the java and protobuf.python backends at once causes a rule graph error #12889

Closed
stuhood opened this issue Sep 14, 2021 · 5 comments · Fixed by #12966
Closed
Assignees
Labels

Comments

@stuhood
Copy link
Sponsor Member

stuhood commented Sep 14, 2021

Enabling both the java and protobuf.python backends at once causes a completely inscrutable rule graph error. This has been the case since #12781, which is when the @rules were first exposed as a backend.

Repro:

./pants --backend-packages=pants.backend.codegen.protobuf.python --backend-packages=pants.backend.experimental.java help
@Eric-Arellano
Copy link
Contributor

When fixing, we should probably expand test_load_backends_integration.py to start testing every combination of backends, which would have caught this. I shudder at the thought of users experiencing this in the wild.

@Eric-Arellano
Copy link
Contributor

When fixing, we should probably expand test_load_backends_integration.py to start testing every combination of backends, which would have caught this.

Apparently I forgot my discrete math / stats classes 😅 we have 24 backends currently. If we just wanted to test every combination where r=12, that's 24! / (12! (24 - 12)!) == 2_704_156. So, not happening...I tried adding tests where r=2, but it did not pick up this failure.

Testing every combination is not feasible. At the same time, only no backends + all backends + each backend on its own seems too limited. Perhaps we randomly select an additional n combinations to test, say 100? Normally random is bad in tests due to flakiness, but here, we would intentionally have it be random so that over time we get better coverage. That's still pretty dang limited in coverage, but better than nothing? cc @benjyw , iirc you enjoy questions like this

@stuhood
Copy link
Sponsor Member Author

stuhood commented Sep 17, 2021

I'm hopeful that fixing the underlying rule graph issue will make testing every combination unnecessary. But we'll see.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Sep 17, 2021 via email

@stuhood
Copy link
Sponsor Member Author

stuhood commented Sep 17, 2021

I'm fairly optimistic that I have an idea of how to do this. Opened #12934.

@stuhood stuhood linked a pull request Sep 21, 2021 that will close this issue
stuhood added a commit that referenced this issue Sep 21, 2021
As described in #12934, we would like plugins to have better defined interfaces, and to further clarify what is available while satisfying a `@union`.

Without making any API changes for `@rule` authors, we can take a first step in this direction by implementing the `Get`s for `@union` types using `Query`s instead. The effect of this is that a `@union` usage may _only_ use the explicitly provided `Param`, and no others. When compared to the usual usage of `Get` (which can consume any `Param`s which are in scope at the "call site"), this makes for a much better defined API boundary.

In order to complete #12934, we will likely want to make interface changes to allow more than the single `@union`-member-`Param` specified to the `Get` to be consumed by a plugin (see the examples in that issue's description). But that is not necessary today, and this change also has the benefit of fixing the blocking issue behind #12889 and #12929 by significantly simplifying the rule graph computation (since plugin boundaries are now "hardcoded", and don't need the `Param` detection executed for `Get`s).

[ci skip-build-wheels]
stuhood added a commit that referenced this issue Sep 21, 2021
With #12889 fixed, re-enable the Java backend in `pantsbuild/pants`.

This reverts commit d471131.

[ci skip-build-wheels]
[ci skip-rust]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants