-
-
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
Performance tuning for inference during test iteration #10062
Comments
This comment has been minimized.
This comment has been minimized.
I've highjacked this one to reference a particular case that is important: switching between For example: running:
and then:
still causes the following rules to run the second time:
Looking at the rule graph for these (generated with So this potentially relates to #10360, in that we might want to move more of options parsing into |
Relates to #6845 (comment): the |
Bumped this one up to the RC: will aim to only tackle the quick fix here, unless someone thinks we should try to tackle both #10360 and this one together. |
I think #10360 can wait for now. Let's get the quick fix in and see how much that fixes. |
Went on a tangent to write #10793 and open #10798. I'm unsure how much of a performance issue #10798 is in practice, so I'm going to experiment with removing |
I have a prototype of this working, with two or three small patches to clean up and land. Should be able to finish by EOD Monday. |
### Problem In order to resolve #10062, we're planning to remove the `OptionsBootstrapper` `Param`, and instead compute it using an uncacheable `@rule`. Doing so performantly relies on successfully "cleaning" (see #10798) all of the nodes above options parsing in cases where the output has not changed, but experimentation and inspection of the types involved in [construct_scope_*](https://github.com/pantsbuild/pants/blob/dadbcc5b7cb08493ff8f5b0c7f88e896e20a7d7e/src/python/pants/option/optionable.py#L40-L64) and [scope_options](https://github.com/pantsbuild/pants/blob/dadbcc5b7cb08493ff8f5b0c7f88e896e20a7d7e/src/python/pants/engine/internals/options_parsing.py#L23-L37) showed that there were missing equals implementations that prevented cleaning. ### Solution Give `Subsystem` and `OptionsValueContainer` structural implementations of `__eq__`. Because they will only be used as return values / positional args (ie, the rust side `Value` struct), they don't need `__hash__`. Additionally, remove `--parent-build-id`, which is not currently consumed for metrics, and which causes lots of invalidation, because every run ends up with new options values. If this facility needs to be revived, it's likely that it should be set only in the context of the `InteractiveProcess` intrinsic, rather than globally... and perhaps not using the options system at all. ### Result Together with #10814 and another PR to follow, `Subsystem` nodes above an uncacheable `@rule` that computes the `OptionsBootstrapper` can be successfully cleaned, which causes early cutoff that allows the rest of the graph to be cleaned with a minimum number of nodes re-running.
The remaining work for this is moving the At a semantic level, we want to make the From an end-user API perspective (e.g.: So I'm thinking that instead of a magical # installed for a particular type
def PerSessionValueRule(typ: Type) -> Rule: ..
# set in the session constructor, and available to be used in `requests` made using the session
session = scheduler.new_session(.., OptionsBootstrapper.create(..))
# may consume the per-session type without it affecting the identity of the request
session.request(MyGoal, Specs(..)) A benefit to this sketch is that it helps to differentiate the identity of your request (the A downside is that this might complicate usage of the |
I like this modeling.
Right now, you can change the args dynamically in a test for the exact same Do you expect that instead the options would be similar to a "bootstrap option", that you have to create a new |
You'd need a new
...which would return a copy of the |
That's what I was thinking with a classmethod like that. But would you then have to recreate all your test setup, like I wonder if we could do something like:
|
Just depends on what you want to do with the API. I would not expect It's also possible that because some of the other methods of
|
Cool. If we could keep the buildroot around, then that sounds like an acceptable API to me. In many ways, more explicit with how to pass args, rather than "sometimes you can include this in your |
…er than a Param (#10827) ### Problem As described on #10062: any change to options values (including the CLI specs and passthrough args) currently completely invalidate all `@rules` that consume `Subsystem`s, because the "identities" (memoization keys) of the involved `@rules` change. As more heavy lifting has begun to depend on options, this has become more obvious and problematic. ### Solution As sketched in #10062 (comment), move to providing the `OptionsBootstrapper` (and in future, perhaps much smaller wrappers around the "args" and "env" instead) via a new uncacheable `SessionValues` intrinsic. More generally, the combination of `Params` for values that _should_ affect the identity of a `@rule`, and `SessionValues` for values that should _not_ affect the identity of a `@rule` seems to be a sufficient solution to the problem described on #6845. The vast majority of values consumed by `@rule`s should be computed from `Params`, so it's possible that the env/args will be the only values we ever provide via `SessionValues`: TBD. ### Result The case described in #10062 (comment) no longer invalidates the consumers of `Subsystem`s, and in general, only the `Subsystem`s that are affected by an option change should be invalidated in memory (although: #10834). Fixes #10062 and fixes #6845. [ci skip-build-wheels]
Specifically, we want to optimize the:
test
-rerunThe text was updated successfully, but these errors were encountered: