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

Subsystems have access to the global options #10834

Closed
stuhood opened this issue Sep 22, 2020 · 17 comments
Closed

Subsystems have access to the global options #10834

stuhood opened this issue Sep 22, 2020 · 17 comments
Labels

Comments

@stuhood
Copy link
Member

stuhood commented Sep 22, 2020

Because Options.for_scope includes the global options, when we instantiate a Subsystem, it will include the global options. This causes unnecessary invalidation of @rules that depend on Subsystems when global options are changed.

stuhood added a commit that referenced this issue Sep 22, 2020
…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]
@stuhood stuhood added the bug label Sep 22, 2020
@Eric-Arellano
Copy link
Contributor

Fixing this will allow us to more robustly fix #10950. Right now, the fix is broken for subscopes. That's fine for now because we don't have them yet in 2.x, but a better fix would be for Options.for_scope() to no longer include global options.

@cognifloyd
Copy link
Member

cognifloyd commented Aug 5, 2021

Copying some info I posted in slack:
I just hit this when attempting to get a list of all of the options that were explicitly added for my subsystem. .options.get_explicit_keys() is almost there, but it includes the pants stuff making it difficult for me to rely on.
In my case, get_explicit_keys() is returning this when none of the subsystem options have been specified (ie stuff in my pants.toml file):
['backend_packages', 'plugins', 'pants_version', 'pants_bin_name', 'pants_workdir', 'pants_supportdir', 'pants_distdir', 'pythonpath', 'pants_ignore']

Fixing this would be really helpful. Please and thank you!

@benjyw
Copy link
Contributor

benjyw commented Aug 6, 2021

For background, there are two related concepts here:

  1. Every scope inherits the keys and values from its enclosing scope, recursively. This was for convenience, so you could access global options without needing to find an instance of them.
  2. If an option is registered with recursive=True then it can also be set to a different value on a subscope. The canonical use-case in v1 was allowing setting --level and --color on a per-task basis. But the global->goal->task hierarchy doesn't really make sense in v2.

I propose eliminating both of these concepts, as they are a little difficult to untwine from each other. And they add a lot of complexity both in the code and to users, for very little gain. If we do want to register a handful of options on, say, all goal options, there are other ways to do so.

Thoughts?

@stuhood
Copy link
Member Author

stuhood commented Aug 6, 2021

I propose eliminating both of these concepts, as they are a little difficult to untwine from each other. And they add a lot of complexity both in the code and to users, for very little gain. If we do want to register a handful of options on, say, all goal options, there are other ways to do so.

That would be lovely.

@Eric-Arellano
Copy link
Contributor

Agreed, that'd be great. If we find we need to add these mechanisms back, I suggest we would want to implement it a different way anyways.

@benjyw
Copy link
Contributor

benjyw commented Aug 6, 2021

Alright, on it... :)

@benjyw
Copy link
Contributor

benjyw commented Aug 6, 2021

So possibly the entire concept of the scope hierarchy is no longer valid? For example, we currently check for "shadowing" - that if we have --foo on an outer scope (e.g., the global scope) we don't also have a --foo on an inner scope. But this seems unnecessary now?

@stuhood
Copy link
Member Author

stuhood commented Aug 6, 2021

So possibly the entire concept of the scope hierarchy is no longer valid? For example, we currently check for "shadowing" - that if we have --foo on an outer scope (e.g., the global scope) we don't also have a --foo on an inner scope. But this seems unnecessary now?

So, while recursive=True options and inheritance of the global scope aren't necessary, I'm still on the fence about whether "scoped Subsystems" will become a thing again. There are some potential usecases in #7735 and #10156 (a subsystem scoped by the "machine/environment" it will be used with, or by where a process will run, respectively).

@Eric-Arellano
Copy link
Contributor

@stuhood what are the odds that we would implement that differently if we decide to add back "scoped subsystems"?

@stuhood
Copy link
Member Author

stuhood commented Aug 6, 2021

@stuhood what are the odds that we would implement that differently if we decide to add back "scoped subsystems"?

That's a question for Benjy I think.

EDIT: One thing I can say though is that the larger the Rust implementation of this becomes, the more we will want to unify them using whichever model is most natural on the rust side: https://github.com/pantsbuild/pants/tree/main/src/rust/engine/client/src/options cc @jsirois

@benjyw
Copy link
Contributor

benjyw commented Aug 6, 2021

Yeah, I think we would want a more deliberate way of saying "there are multiple versions of these options" than just implying it by name scoping.

@benjyw
Copy link
Contributor

benjyw commented Aug 6, 2021

That is, we can generate multiple "copies" of the same subsystem easily, with a "base subsystem" that they can get defaults from, and give them each a scope, and the scopes may or may not form a naming hierarchy via ., but I don't want those to be intertwined as they are now.

@benjyw
Copy link
Contributor

benjyw commented Aug 6, 2021

pre-work for this: #12514

@benjyw
Copy link
Contributor

benjyw commented Aug 7, 2021

Addressed in #12519

@benjyw
Copy link
Contributor

benjyw commented Aug 7, 2021

Closed in #12519

@benjyw benjyw closed this as completed Aug 7, 2021
@benjyw
Copy link
Contributor

benjyw commented Aug 7, 2021

@cognifloyd This should go out in the next 2.7.0 dev release, in about a week.

@cognifloyd
Copy link
Member

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants