@union / UnionRule for letting the engine figure out paths to products not known in advance by cosmicexplorer · Pull Request #7116 · pantsbuild/pants

cosmicexplorer · 2019-01-20T21:14:10Z

Problem

Resolves #4535.

It's currently difficult for rules to tell the engine "give me an instance of y from this x according to the registered rules in the rule graph" if x is not a specific type known in advance (see #4535 for use cases for this). The alternative is that upstream rules have to use conditional logic outside of the engine, which is very difficult to write in a way that combines with arbitrary rulesets from downstream plugins, and code written to get around this can be error-prone. This is sad because the rule graph is generated by combining rules in pants core as well as in plugins, so all the necessary information is there, we just can't make use of it.

This PR introduces the @union decorator and UnionRule type to allow for static resolution of "union" types in yield Get(Product, UnionType, subject) statements as per this comment in #4535 and below.

Solution

Python

Add a subject_declared_type field to pants.engine.selectors.Get instead of inferring it from the subject type (the 2-argument form of Get is still supported).
Introduce Get.create_statically_from_rule_graph() classmethod to make it clear when the Get subject is populated or not.
Introduce @union and UnionRule to describe union types which can be requested with a yield Get(Product, UnionType, subject) in rule bodies (as per this comment on #4535).
- Note that @union classes are not registered in def rules(): ... -- this distinction seems to make sense as union classes are never instantiated.
Create a really simple union_rules dict field in RuleIndex which registers @union_rule() decorators as a map from union base type -> [union members].
Propagate the union_rules dict to the scheduler, and when adding Get edges (in _register_task() in scheduler.py), check if the subject_declared_type is a union base, and if so add edges to all union members.
Create a HydrateableField union base which is used to resolve (for now) either SourcesField or BundlesField in a yield Get().

Result

Users can now dynamically add union members in plugins and backends to be processed by upstream rules using yield Get(...) which don't know anything about them, and with a static universe of known union members which the engine uses uses to type-check the subject of a yield Get(...) at rule execution time.

cosmicexplorer · 2019-01-21T04:27:17Z

Fixed the unit test errors by moving Get typechecking to scheduler.py in 1dd0377 because we do that during rule execution now, and also added testing for Get.extract_constraints().

./pants --v2 --no-v1 list :: fails (see travis) which I will fix momentarily.

cosmicexplorer · 2019-01-21T06:08:07Z

./pants --v2 --no-v1 list :: fails (see travis) which I will fix momentarily.

Fixed and added some argument checking in 78e5c31 to overcome the otherwise-ambiguous reasoning about what means what in Get() with 2 vs 3 args.

stuhood

Thanks! This looks neat. I think it's possible to make it simpler, and it would be good to see some actual usage within this PR.

stuhood · 2019-01-28T02:51:38Z

tests/python/pants_test/engine/test_scheduler.py

  yield D(b)


+@union


Even seeing it sketched out, it's not quite clear what real world usage will look like. Would you mind updating the BundleField/SourceField usecase

pants/src/python/pants/engine/legacy/graph.py

Lines 499 to 517 in f79b62b

class HydratedField(datatype(['name', 'value'])):

"""A wrapper for a fully constructed replacement kwarg for a HydratedTarget."""

@rule(HydratedTarget, [Select(TargetAdaptorContainer)])

def hydrate_target(target_adaptor_container):

target_adaptor = target_adaptor_container.value

"""Construct a HydratedTarget from a TargetAdaptor and hydrated versions of its adapted fields."""

# Hydrate the fields of the adaptor and re-construct it.

hydrated_fields = yield [(Get(HydratedField, BundlesField, fa)

if type(fa) is BundlesField

else Get(HydratedField, SourcesField, fa))

for fa in target_adaptor.field_adaptors]

kwargs = target_adaptor.kwargs()

for field in hydrated_fields:

kwargs[field.name] = field.value

yield HydratedTarget(target_adaptor.address,

TargetAdaptor(**kwargs),

tuple(target_adaptor.dependencies))

here, to see how that informs the rest of the PR?

Doing this part first after rebasing!

Did this and it seemed to mysteriously work? It essentially just let us remove the if in this comprehension you've highlighted.

Looks great!

One possibility (not sure whether it is an improvement though...) would be to do a function call rather than a decorator for union members.

That might look like:

@union class HydrateableField(object): pass class SourcesField(...): ... def rules(): return [ union_rule(HydrateableField, SourcesField) ]

There ended up being a ridiculous amount of confusion when trying to support use of @union_rule as a decorator and as a function call, so I instead converted it to a datatype UnionRule like we do with e.g. RootRule, without any decorators, which makes much more sense anyway. I just didn't want to drop the possibility of it being a decorator too early.

stuhood · 2019-01-28T03:06:02Z

src/python/pants/engine/native.py

      return Function(self.context.to_key(constraint))
    def tc(constraint):
      return TypeConstraint(self.context.to_key(constraint))
+    def flatten_type_map_to_constraints(type_constraints_map):


I think that part of the elegance of the implementation you suggested is that it shouldn't be necessary for the native engine to know about the union (yet... we could decide to do that later)...

If at RuleIndex (or perhaps Scheduler?) creation time you:

# expand a Get for a Union type... Get(TypeX, Union) # ...into Gets for all of its members: Get(TypeX, UnionMember1), Get(TypeX, UnionMember2), .. #etc

... then there is no need for the engine itself to know about the existence of the union.

This is really good.

I'm actually thinking now that we don't need to do any expansion, but rather just check all Gets for union types (I believe we can/should do this at RuleIndex creation time) are valid according to the registered union rules (not sure if this was your intended meaning). This gives us the guarantee you've described below of knowing that Gets are valid statically, which allows the engine to just generate a Get from the runtime type as usual. And that should allow us to remove any knowledge of unions from the engine.

I'm actually thinking now that we don't need to do any expansion, but rather just check all Gets for union types are valid according to the registered union rules

Not quite: I think all I was referring to was replacing each union-Get with all union-member-Gets. Assuming you do that, the RuleGraph handles all validation for you.

I believe we can/should do this at RuleIndex creation time

I don't know if we can assume that a RuleIndex always represents a closed world (right now). But we do know that we have a closed world at Scheduler creation time... so at Scheduler creation time you could rewrite the contents of the RuleIndex to replace a union-Get with union-member-Gets?

After diving into the implementation of removing all the union knowledge from the engine, I think that sounds correct (since that's where we're already linking up the Gets for unions in this PR anyway), and actually may already be done.

It was! It seems like all the engine work was purely to provide a more specific error message on failing to resolve a union somehow. I did also convert a couple panic!s into throw()s, which enabled the technique you describe in another comment (just making a Get and letting the engine error out if there's no path).

stuhood · 2019-01-28T03:16:40Z

src/rust/engine/src/externs.rs

-        interns.insert(response.values.unwrap_one()),
-      )))
+      let product = TypeConstraint(interns.insert(response.products.unwrap_one()));
+      let subject_declared_type =


Hm. So I guess this is because at runtime (rather than at @rule parse time), we'll see a Get with a declared type that might be a union, and then we might want to know the members of the union in order to validate that the given value is a member of that union?

It feels like not actually checking the declared type here, and just using exactly the type of the subject value would be fine. The result would still be the same as if there had been multiple literal Gets in the body of the @rule... and by this point we would already have validated that those are correct in the rule graph.

AFAICT, that would mean this method would not need to change at all?

If I'm parsing your comment correctly, after expanding the Gets to include all the registered constituent union members as you described in another comment, this method would indeed not need to change.

I have implemented this, all I had to do was make gen_get() return an error instead of panicking!

stuhood

Thanks! This looks really great.

stuhood · 2019-03-03T17:35:31Z

src/python/pants/engine/rules.py

+
+    def add_type_transition_rule(union_rule):
+      """
+      NB: This does not require that union bases be supplied to `def rules():`! not sure if that's


Totally fine with that, for the same reason you put in the description.

Have moved this to a comment noting that this is because the union type is never instantiated!

stuhood · 2019-03-03T17:42:01Z

tests/python/pants_test/engine/test_scheduler.py

+  @contextmanager
+  def _assert_execution_error(self, expected_msg):
+    # TODO(#7303): use self.assertRaisesWithMessageContaining()!
+    with self.assertRaises(ExecutionError) as cm:


Could this be self.assertRaisesRegexp?

It might be, but I'm realizing now that this probably would be difficult to convert to self.assertRaisesWithMessageContaining() anyway because of the way it preprocesses the exception message. That could be converted to a regexp match (which I tried to do at one point), but I think stripping the locations from the traceback is much more robust and was just going to remove this comment, actually.

stuhood · 2019-03-03T17:42:28Z

src/python/pants/engine/legacy/graph.py

-  hydrated_fields = yield [(Get(HydratedField, BundlesField, fa)
-                            if type(fa) is BundlesField
-                            else Get(HydratedField, SourcesField, fa))
+  hydrated_fields = yield [Get(HydratedField, HydrateableField, fa)


cosmicexplorer · 2019-03-03T19:25:26Z

Closed #7117 since the engine doesn't know about union rules anymore!

…nto the engine

… reason

also test Get.extract_constraints()!

…d to test failure

illicitonion

Woo!

illicitonion · 2019-03-05T02:21:38Z

src/python/pants/engine/rules.py

          raise TypeError("Expected callable {} to be decorated with @rule.".format(entry))
        add_rule(rule)
      else:
+        # TODO: update this message!


Either do this, or flesh out the TODO explaining what update should happen.

Oops! Will do!

cosmicexplorer force-pushed the union-rules-idk branch from c57bfc7 to 166beac Compare January 20, 2019 22:10

cosmicexplorer requested a review from stuhood January 20, 2019 22:36

cosmicexplorer changed the title ~~[WIP] @union_rule (or something along those lines)~~ @union_rule (or something along those lines) Jan 20, 2019

cosmicexplorer requested a review from illicitonion January 20, 2019 22:38

cosmicexplorer mentioned this pull request Jan 20, 2019

represent union types in the graph visualizer somehow #7117

Closed

cosmicexplorer force-pushed the union-rules-idk branch from 1dd0377 to 78e5c31 Compare January 21, 2019 06:06

cosmicexplorer force-pushed the union-rules-idk branch from 78e5c31 to ac37919 Compare January 21, 2019 21:48

cosmicexplorer mentioned this pull request Jan 25, 2019

remote::tests::retry_for_cancelled_channel is flaky #7149

Closed

stuhood reviewed Jan 28, 2019

View reviewed changes

cosmicexplorer force-pushed the union-rules-idk branch from ac37919 to c145847 Compare March 3, 2019 02:03

cosmicexplorer changed the title ~~@union_rule (or something along those lines)~~ @union / UnionRule for letting the engine figure out paths to products not known in advance Mar 3, 2019

stuhood approved these changes Mar 3, 2019

View reviewed changes

cosmicexplorer added 15 commits March 3, 2019 14:16

separate the declared type from the actual subject type of the Get

18bfd47

plumb a dict of type constraint -> list<type constraint> for unions i…

6fa19b8

…nto the engine

add testing for union rules which fails

7f24f9f

union rules work now

709e2dc

introduce Get.create_statically_for_rule_graph() to avoid confusion

affab03

introduce a wrapper datatype for normalized_rules() for no particular…

78f87fb

… reason

flesh out docstrings

eea0f6f

add union_rules to all Scheduler() constructions

fdda871

add followup issue link for graph visualization

4aebfc4

test the Get typechecking which we now do during rule execution

23ca41b

also test Get.extract_constraints()!

add further checking to Get() arguments to overcome ambiguity that le…

3f48ac5

…d to test failure

make bundles and sources fields use union rules

872921a

remove all knowledge of union rules from the engine

cc421d4

turn panics into throws and make union tests pass!

eb928c1

remove @union_rule for UnionRule() datatype and add docstrings

07f5835

cosmicexplorer added 2 commits March 3, 2019 14:16

cleanup in response to review comments

628de1c

remove now-unused native lib helper

93e2f4e

cosmicexplorer force-pushed the union-rules-idk branch from a247b85 to 93e2f4e Compare March 3, 2019 22:25

illicitonion approved these changes Mar 5, 2019

View reviewed changes

This was referenced Mar 5, 2019

consider making a defaultdict-like wrapper for OrderedDict #7311

Closed

convert usages of TypeConstraint to TypeId for rule products in the engine #7114

Merged

cosmicexplorer added 2 commits March 4, 2019 20:32

flesh out the error message TODO!

8d4eb55

fix error message test

5d58801

stuhood merged commit e8ccd90 into pantsbuild:master Mar 6, 2019

stuhood mentioned this pull request Aug 15, 2019

Design v2 Target API #4535

Closed

	class HydratedField(datatype(['name', 'value'])):
	"""A wrapper for a fully constructed replacement kwarg for a HydratedTarget."""


	@rule(HydratedTarget, [Select(TargetAdaptorContainer)])
	def hydrate_target(target_adaptor_container):
	target_adaptor = target_adaptor_container.value
	"""Construct a HydratedTarget from a TargetAdaptor and hydrated versions of its adapted fields."""
	# Hydrate the fields of the adaptor and re-construct it.
	hydrated_fields = yield [(Get(HydratedField, BundlesField, fa)
	if type(fa) is BundlesField
	else Get(HydratedField, SourcesField, fa))
	for fa in target_adaptor.field_adaptors]
	kwargs = target_adaptor.kwargs()
	for field in hydrated_fields:
	kwargs[field.name] = field.value
	yield HydratedTarget(target_adaptor.address,
	TargetAdaptor(**kwargs),
	tuple(target_adaptor.dependencies))

Uh oh!

Conversation

cosmicexplorer commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Python

Result

Uh oh!

cosmicexplorer commented Jan 21, 2019

Uh oh!

cosmicexplorer commented Jan 21, 2019

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuhood Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer commented Mar 3, 2019

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cosmicexplorer commented Jan 20, 2019 •

edited

Loading

stuhood Mar 3, 2019 •

edited

Loading

cosmicexplorer Mar 3, 2019 •

edited

Loading

cosmicexplorer Mar 3, 2019 •

edited

Loading