Eliminate most rule registration boilerplate. #10477
Conversation
|
1st commit is the technology, 2nd commit uses it. |
jsirois
force-pushed
the
rule/register_ergonomics
branch
from
0f26d8b
to
74ee87b
Compare
|
OK - Rebased and de-conflicted. PTAL. |
| @@ -79,4 +79,4 @@ async def create_awslambda( | |||
|
|
|||
|
|
|||
| def rules(): | |||
| return [create_awslambda] | |||
| return register_rules() | |||
There was a problem hiding this comment.
Thoughts on inverting this, and calling it from outside the module? Ie, having an importer call with the module name:
register_rules(pants.backend.awslambda.common.awslambda_common_rules)
Would have two advantages I think:
- no uncertainty about what happens when called with no args (although that's not too magical)
- no need for a convention inside of each module to export rules: things are mostly consistent, but avoiding the need for the convention entirely would be great.
It looks like to make it work though, UnionRule could maybe move to being a class decorator... ie:
@union
class MyUnion: pass
@union_member(MyUnion)
class MyUnionMember: pass
...so maybe better as a followup step.
There was a problem hiding this comment.
Thoughts on inverting this, and calling it from outside the module?
The API supports it and that was my intention, but its not easy right now so took this smaller step.
Since we hack RootRules in to support tests in our production rules() lists (not cool), using this function from outside the module does not catch those and foces you to register those.
It looks like to make it work though, UnionRule could maybe move to being a class decorator... ie:
I have an old branch that instead just forces union members to be subclasses of the @union decorated (potentially just marker) type. That will kill all member boilerplate - you just subclass which should feel like it makes sense to rule authors opting into the test goal say from a pure Python perspective, Pants engine aside.
There was a problem hiding this comment.
I like the idea of @union_member, although I don't think that would work with plugin fields: #10469
I also don't think that we want plugin fields to have to subclass the target they're being used for. A key part of the Target API is that a single field definition may be used across multiple targets. Also, fields already subclass a field template. So, we would end up with:
class Example(IntField, PythonLibrary.PluginField, PythonTests.PluginField):
alias = "example"There was a problem hiding this comment.
PluginField should not be using Union IMO. If we instead Target.register_plugin_field(Field) -> Rule then Rule can be whatever type we want and e don't need to bastardize UnionRule. Hopefully everyone realizes that by bastardizing Unions in this way, if you look at the action graph you get spammed with a bunch of fake entries for non-rule edges.
There was a problem hiding this comment.
Union has a very specific purpose: allow a Get to retrieve a given product type from an unknown provider - ie a rule set forthcoming that can provide that product type given an unknown input type at the time of writing the top-level goal. Thats pretty darn specific. Not everything is a union, just that.
src/python/pants/engine/rules.py
Outdated
| implicitly form a loosely coupled RuleGraph: this facility exists only to assist with | ||
| boilerplate removal. | ||
| """ | ||
| def register_rules(*namespaces: Union[ModuleType, Mapping[str, Any]]) -> Iterable[Rule]: |
There was a problem hiding this comment.
It's unlikely to be the slowest part of startup, but none of our calls to def rules are memoized. Perhaps the inner portion of this method (post module identification) could be.
There was a problem hiding this comment.
Yeah. The fact that we re-export rules at all / need to seems problematic. I have not thought through why we need to do that which is what leads to the need for this optimization now IIUC. It would be great to kill that need instead / in addition.
There was a problem hiding this comment.
Mm. There was some discussion of this the other day: https://pantsbuild.slack.com/archives/C0D7TNJHL/p1595565944292800 ... but yea, unclear what to do, as it does seem to make things easier in some cases.
|
One other bit: this is more like |
+1 |
|
Agreed - went with collect_rules. |
Introduce a register_rules function that can find and register all @rules and the SubsystemRules they depend on. This can be used to both reduce boilerplate and also make rule development less surprising. Using register_rules it now becomes rarer to edit rules and encounter errors due to forgetting to register rules for the various introduced subsystems and @rule functions. Also streamline the Rule interface and the `def rules()` semantics; now all rules are Rules save for UnionRule which can be dealt away with later to leave users only exposed to registering Rules. [ci skip-rust-tests]
[ci skip-rust-tests]
# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
# 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]
jsirois
force-pushed
the
rule/register_ergonomics
branch
from
c909a19
to
bd20fcb
Compare
Introduce a register_rules function that can find and register all
@rules and the SubsystemRules they depend on. This can be used to
both reduce boilerplate and also make rule development less surprising.
Using register_rules it now becomes rarer to edit rules and encounter
errors due to forgetting to register rules for the various introduced
subsystems and @rule functions.
Also streamline the Rule interface and the
def rules()semantics;now all rules are Rules save for UnionRule which can be dealt away
with later to leave users only exposed to registering Rules and allow
def rules() -> Iterable[Rule]:.[ci skip-rust-tests]