OptionsBootstrapper is provided via a new SessionValues facility rather than a Param #10827

stuhood · 2020-09-21T19:03:00Z

Problem

As described on #10062: any change to options values (including the CLI specs and passthrough args) currently completely invalidate all @rules that consume Subsystems, 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 @rules 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 Subsystems, and in general, only the Subsystems that are affected by an option change should be invalidated in memory (although: #10834).

Fixes #10062 and fixes #6845.

[ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

…Session, and is uncacheable. [ci skip-build-wheels]

[ci skip-build-wheels]

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood · 2020-09-22T23:24:08Z

src/python/pants/testutil/rule_runner.py

+        # TODO: Update all callsites to pass this explicitly via session values.
+        session = self.scheduler
+        for value in inputs:
+            if type(value) == OptionsBootstrapper:
+                session = self.scheduler.scheduler.new_session(
+                    build_id="buildid_for_test",
+                    should_report_workunits=True,
+                    session_values=SessionValues({OptionsBootstrapper: value}),
+                )


Note: because this hack is quite tiny, I punted on actually changing the TestBase and RuleRunner APIs yet to allow for passing/setting SessionValues. If folks are ok with doing this as a followup, that would be my preference.

What's the plan for when to followup? My concern is our documentation; we have users who are starting to write tests for their plugins. We say it's Alpha and we'll make changes, but still helpful to fix before too much code is written.

Fwit, I'm happy to help with this fix.

stuhood · 2020-09-22T23:25:28Z

Commits should be reviewed independently.

Eric-Arellano · 2020-09-22T23:25:08Z

src/python/pants/backend/awslambda/python/awslambda_python_rules_test.py

-            QueryRule(
-                CreatedAWSLambda, (PythonAwsLambdaFieldSet, OptionsBootstrapper, PantsEnvironment)
-            ),
+            QueryRule(CreatedAWSLambda, (PythonAwsLambdaFieldSet, PantsEnvironment)),


Should PantsEnvironment become a SessionValue in a followup?

Yes it should!

That'll be wonderful to not have to have users need to fiddle with this during test setup. And an API like this would be nice and declarative:

rule_runner.set_env_var("FOO", "val")

Eric-Arellano · 2020-09-22T23:28:01Z

src/python/pants/engine/internals/options_parsing.py

+    # TODO: Once the OptionsBootstrapper has been removed from all relevant QueryRules, this lookup
+    # should be extracted into a separate @rule.


It looks like they are all removed. Is this still accurate?

This can be fixed now: will push another commit.

Since this is green, will follow up to fix this first thing tomorrow.

Eric-Arellano · 2020-09-22T23:29:53Z

src/python/pants/engine/session.py

@@ -0,0 +1,19 @@
+# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).


Should this be in engine/internal? The question is mostly if we expect to document the type SesssionValues and we expect plugin authors to directly import the value. Generally, this gets abstracted from them fwict.

It might depend what API we end up with for RuleRunner/TestBase...? I guess it's unlikely that the actual SessionValues type would be exposed in that API though.

I guess it's unlikely that the actual SessionValues type would be exposed in that API though.

Yeah, I suspect it's too low level. We can also always "promote" the API to be public if we realize it's relevant. It's harder to demote it to private.

Eric-Arellano · 2020-09-22T23:31:20Z

src/python/pants/testutil/rule_runner.py

+        # TODO: Update all callsites to pass this explicitly via session values.
+        session = self.scheduler
+        for value in inputs:
+            if type(value) == OptionsBootstrapper:
+                session = self.scheduler.scheduler.new_session(
+                    build_id="buildid_for_test",
+                    should_report_workunits=True,
+                    session_values=SessionValues({OptionsBootstrapper: value}),
+                )


What's the plan for when to followup? My concern is our documentation; we have users who are starting to write tests for their plugins. We say it's Alpha and we'll make changes, but still helpful to fix before too much code is written.

Fwit, I'm happy to help with this fix.

Eric-Arellano · 2020-09-22T23:31:55Z

src/python/pants/testutil/rule_runner.py

+        # TODO: Update all callsites to pass this explicitly via session values.
+        session = self.scheduler
+        for value in inputs:
+            if type(value) == OptionsBootstrapper:


should you then pop this value from the inputs?

Yea, you could. It's unused now.

Eric-Arellano · 2020-09-22T23:33:59Z

Thanks! I'm excited for the improvement from this change. The overall design looks good, including your sketch for the RuleRunner API.

As of #10827, options are now set up through a new `SessionValues` mechanism. This means that our tests should no longer be passing `OptionsBootstrapper` as a `Param` anymore. Instead, we must mutate the `SchedulerSession` when setting options. We surface a new entry-point `.set_options()`, which will change the options for the life of the `RuleRunner` instance. ```python rule_runner.set_options(["--pants-ignore=['foo']"]) ``` `RuleRunner` already was high on mutation, such as `rule_runner.create_file()` mutating the build root. This is why we encourage using Pytest fixtures to ensure each test is a new instance. A followup will move `PantsEnvironment` to a `SessionValue`. [ci skip-rust] [ci skip-build-wheels]

As introduced in #10827, this is a more correct modeling for reading the environment variables. It will result in less invalidation of Pantsd. Likewise, we can extend our `rule_runner.set_options()` method introduced in #10859 to simplify how tests change the `PantsEnvironment`. [ci skip-rust] [ci skip-build-wheels]

stuhood changed the title ~~OptionsBootstrapper is constructed from an uncacheable singleton rather being a param~~ OptionsBootstrapper is constructed from an uncacheable singleton rather being a Param Sep 21, 2020

stuhood mentioned this pull request Sep 21, 2020

Performance tuning for inference during test iteration #10062

Closed

stuhood added 2 commits September 21, 2020 15:54

Simplify rule registration a bit.

d083bdd

[ci skip-rust] [ci skip-build-wheels]

Add a SessionValues intrinsic that lifts a value set on the Scheduler…

a91ba7f

…Session, and is uncacheable. [ci skip-build-wheels]

stuhood force-pushed the stuhood/ob-as-uncacheable branch from 63f29c7 to 434bcfe Compare September 22, 2020 21:00

Fetch the OB from SessionValues.

a40a6b6

[ci skip-build-wheels]

stuhood force-pushed the stuhood/ob-as-uncacheable branch from 434bcfe to a40a6b6 Compare September 22, 2020 22:11

Remove the OptionsBootstrapper from all relevant QueryRules.

623a2f9

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood marked this pull request as ready for review September 22, 2020 23:20

stuhood requested review from Eric-Arellano, gshuflin, jsirois and benjyw September 22, 2020 23:20

stuhood commented Sep 22, 2020

View reviewed changes

Eric-Arellano approved these changes Sep 22, 2020

View reviewed changes

stuhood changed the title ~~OptionsBootstrapper is constructed from an uncacheable singleton rather being a Param~~ OptionsBootstrapper is provided via a new SessionValues facility rather than a Param Sep 22, 2020

stuhood merged commit f4763dd into pantsbuild:master Sep 22, 2020

stuhood deleted the stuhood/ob-as-uncacheable branch September 22, 2020 23:43

Eric-Arellano mentioned this pull request Sep 25, 2020

Add RuleRunner.set_options() #10859

Merged

Eric-Arellano mentioned this pull request Sep 25, 2020

Move PantsEnvironment into SessionValues #10862

Merged

stuhood mentioned this pull request Jun 17, 2021

Re-enable concurrent runs for pantsd in v2 #7654

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OptionsBootstrapper is provided via a new SessionValues facility rather than a Param #10827

OptionsBootstrapper is provided via a new SessionValues facility rather than a Param #10827

stuhood commented Sep 21, 2020 •

edited

Loading

stuhood Sep 22, 2020

Eric-Arellano Sep 22, 2020

stuhood commented Sep 22, 2020

Eric-Arellano Sep 22, 2020

stuhood Sep 22, 2020

Eric-Arellano Sep 22, 2020

Eric-Arellano Sep 22, 2020

stuhood Sep 22, 2020

stuhood Sep 22, 2020

Eric-Arellano Sep 22, 2020

stuhood Sep 22, 2020

Eric-Arellano Sep 22, 2020

Eric-Arellano Sep 22, 2020

Eric-Arellano Sep 22, 2020

stuhood Sep 22, 2020

Eric-Arellano commented Sep 22, 2020

		# TODO: Once the OptionsBootstrapper has been removed from all relevant QueryRules, this lookup
		# should be extracted into a separate @rule.

		@@ -0,0 +1,19 @@
		# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).

OptionsBootstrapper is provided via a new SessionValues facility rather than a Param #10827

OptionsBootstrapper is provided via a new SessionValues facility rather than a Param #10827

Conversation

stuhood commented Sep 21, 2020 • edited Loading

Problem

Solution

Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Sep 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Sep 22, 2020

stuhood commented Sep 21, 2020 •

edited

Loading