-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Allow @union
s to declare additional types which are provided to implementers
#16717
Conversation
#16717 increases the cost of rule graph solving by about 50%: profiling showed that hashing and allocation contribute a significant amount of time there. The default `SipHasher` used in Rust is secure and randomized, but more expensive than a simpler hasher such as `FnvHasher` (see https://nnethercote.github.io/perf-book/hashing.html). This change switches to `FnvHasher` in the `rule_graph` crate, and fills out its usage in the `graph` crate. Additionally, `combinations_of_one` generates many tiny `Vec`s of a known fixed size, but was not pre-allocating them at that size. [ci skip-build-wheels]
…ld#16726) pantsbuild#16717 increases the cost of rule graph solving by about 50%: profiling showed that hashing and allocation contribute a significant amount of time there. The default `SipHasher` used in Rust is secure and randomized, but more expensive than a simpler hasher such as `FnvHasher` (see https://nnethercote.github.io/perf-book/hashing.html). This change switches to `FnvHasher` in the `rule_graph` crate, and fills out its usage in the `graph` crate. Additionally, `combinations_of_one` generates many tiny `Vec`s of a known fixed size, but was not pre-allocating them at that size. [ci skip-build-wheels]
3468ccb
to
a281656
Compare
…lize "re-entering" the rule graph via a `Query`. [ci skip-build-wheels]
[ci skip-build-wheels]
[ci skip-build-wheels]
a281656
to
738cc2b
Compare
Sorry for the big change... I'm really looking forward to replacing this rule graph implementation. Commits are useful to review independently, particularly the top one which demonstrates usage. See also #16721 for additional usages. |
@@ -144,7 +149,7 @@ def num_wheels(self) -> int: | |||
|
|||
|
|||
@rule | |||
def car_num_wheels(car: Car) -> int: | |||
def car_num_wheels(car: Car, _: Fuel) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this new syntax, and having a Fuel object be passed into the construction of the union member(s)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be equivalent from a "number of @rule
instances created" perspective.
But from a practical perspective, adjusting every type everywhere to pass a type (and then actually passing it) is precisely the kind of thing the engine was designed to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead of adjusting types and callsites we adjust rules declarations? I can dig it.
let gil = Python::acquire_gil(); | ||
let py = gil.python(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this is deprecated in the newest PyO3. Better is to use with_gil
pass | ||
|
||
|
||
@union(in_scope_types=[Fuel]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in_scope
sounds weird to me. It doesn't quite give that "you have to declare additional arguments" vibe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't quite give that "you have to declare additional arguments" vibe.
To be clear: you don't have to declare any additional arguments. These are just types that are guaranteed to be available to the callee. So in this example, you can see that motorcycle_num_wheels
doesn't use Fuel
. It could, but it doesn't need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to changing the name as part of #16721 (and beyond), so I'm going to roll with this for now.
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
In order to differentiate subgraphs of the `@rule` graph for different platforms and environments for #7735 (to support #13682), the environment that is in use needs to become a very widespread parameter in the `@rule` graph. To do that, this change: 1. Adjusts all production `QueryRule`s to declare that an `EnvironmentName` must be provided. 2. Adjusts most `@union`s to use #16717 to declare that an `EnvironmentName` will be available. 3. Computes the `Platform` from the `EnvironmentName` (currently as a noop, since we don't actually have multiple instances of the `EnvironmentName`). A caveat for point 1 is that to avoid churn, we only update _production_ `QueryRule`s in this change. For continuity in tests, `RuleRunner` will install a singleton `EnvironmentName` by default, and uses a (probably temporary) facility to filter the `EnvironmentName` back out of all `QueryRule`s. Tests that actually want to test with multiple environments should disable that facility.
As described in #12934, each
@union
Get
callsite currently implicitly represents an API where exactly the parameters provided to theGet
are available, and no others.In order for other types to be made available to plugins (most immediately, a new
EnvironmentName
type to differentiate subgraphs for different environments in #13682), we need a new syntax to indicate that@union
s provide additional types as part of their API.To that end, this change allows a
@union
declaration to specify additional types that will be in scope for callees (and consequently must be available to callers):Fixes #12934.
[ci skip-build-wheels]