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

@union / UnionRule for letting the engine figure out paths to products not known in advance #7116

Merged
merged 19 commits into from Mar 6, 2019
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
18bfd47
separate the declared type from the actual subject type of the Get
cosmicexplorer Jan 20, 2019
6fa19b8
plumb a dict of type constraint -> list<type constraint> for unions i…
cosmicexplorer Jan 20, 2019
7f24f9f
add testing for union rules which fails
cosmicexplorer Jan 20, 2019
709e2dc
union rules work now
cosmicexplorer Jan 20, 2019
affab03
introduce Get.create_statically_for_rule_graph() to avoid confusion
cosmicexplorer Jan 20, 2019
78f87fb
introduce a wrapper datatype for normalized_rules() for no particular…
cosmicexplorer Jan 20, 2019
eea0f6f
flesh out docstrings
cosmicexplorer Jan 20, 2019
fdda871
add union_rules to all Scheduler() constructions
cosmicexplorer Jan 21, 2019
4aebfc4
add followup issue link for graph visualization
cosmicexplorer Jan 21, 2019
23ca41b
test the Get typechecking which we now do during rule execution
cosmicexplorer Jan 21, 2019
3f48ac5
add further checking to Get() arguments to overcome ambiguity that le…
cosmicexplorer Jan 21, 2019
872921a
make bundles and sources fields use union rules
cosmicexplorer Mar 3, 2019
cc421d4
remove all knowledge of union rules from the engine
cosmicexplorer Mar 3, 2019
eb928c1
turn panics into throws and make union tests pass!
cosmicexplorer Mar 3, 2019
07f5835
remove @union_rule for UnionRule() datatype and add docstrings
cosmicexplorer Mar 3, 2019
628de1c
cleanup in response to review comments
cosmicexplorer Mar 3, 2019
93e2f4e
remove now-unused native lib helper
cosmicexplorer Mar 3, 2019
8d4eb55
flesh out the error message TODO!
cosmicexplorer Mar 5, 2019
5d58801
fix error message test
cosmicexplorer Mar 5, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

turn panics into throws and make union tests pass!

  • Loading branch information...
cosmicexplorer committed Mar 3, 2019
commit eb928c1e29e14eddacaacd034af30e23cf3853e9
@@ -812,20 +812,21 @@ impl Task {
.core
.rule_graph
.edges_for_inner(&entry)
.expect("edges for task exist.")
.entry_for(&select_key)
.unwrap_or_else(|| {
panic!(
"{:?} did not declare a dependency on {:?}",
entry, select_key
)
})
.clone();
.ok_or_else(|| throw(&format!("no edges for task {:?} exist!", entry)))
.and_then(|edges| {
edges.entry_for(&select_key).cloned().ok_or_else(|| {
throw(&format!(
"{:?} did not declare a dependency on {:?}",
entry, select_key
))
})
});
// The subject of the get is a new parameter that replaces an existing param of the same
// type.
let mut params = params.clone();
params.put(get.subject);
Select::new(params, get.product, entry).run(context.clone())
future::result(entry)
.and_then(move |entry| Select::new(params, get.product, entry).run(context.clone()))
})
.collect::<Vec<_>>();
future::join_all(get_futures).to_boxed()
@@ -175,6 +175,7 @@ def test_transitive_params(self):

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

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 3, 2019

Member

Could this be self.assertRaisesRegexp?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 3, 2019

Author Contributor

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.

yield
self.assertIn(expected_msg, remove_locations_from_traceback(str(cm.exception)))
@@ -186,13 +187,17 @@ def test_union_rules(self):
a, = self.scheduler.product_request(A, [Params(UnionWrapper(UnionB()))])
self.assertTrue(isinstance(a, A))
# Fails due to no union relationship from A -> UnionBase.
expected_msg = "Exception: None of the registered union members matched the subject. declared union type: TypeConstraint(Exactly(UnionBase)), union members: {TypeConstraint(Exactly(UnionA)), TypeConstraint(Exactly(UnionB))}, subject: <pants_test.engine.test_scheduler.A object at 0xEEEEEEEEE>"
expected_msg = """\
Exception: WithDeps(Inner(InnerEntry { params: {UnionWrapper}, rule: Task(Task { product: TypeConstraint(Exactly(A)), clause: [Select { product: Exactly(UnionWrapper) }], gets: [Get { product: TypeConstraint(Exactly(A)), subject: UnionA }, Get { product: TypeConstraint(Exactly(A)), subject: UnionB }], func: Function(<function a_union_test at 0xEEEEEEEEE>), cacheable: true }) })) did not declare a dependency on JustGet(Get { product: TypeConstraint(Exactly(A)), subject: A })
"""
with self._assert_execution_error(expected_msg):
self.scheduler.product_request(A, [Params(UnionWrapper(A()))])

def test_get_type_match_failure(self):
"""Test that Get(...)s are now type-checked during rule execution, to allow for union types."""
expected_msg = "Exception: Declared type did not match actual type for Get { product: TypeConstraint(Exactly(A)), subject: <pants_test.engine.test_scheduler.A object at 0xEEEEEEEEE>"
expected_msg = """\
Exception: WithDeps(Inner(InnerEntry { params: {TypeCheckFailWrapper}, rule: Task(Task { product: TypeConstraint(Exactly(A)), clause: [Select { product: Exactly(TypeCheckFailWrapper) }], gets: [Get { product: TypeConstraint(Exactly(A)), subject: B }], func: Function(<function a_typecheck_fail_test at 0xEEEEEEEEE>), cacheable: true }) })) did not declare a dependency on JustGet(Get { product: TypeConstraint(Exactly(A)), subject: A })
"""
with self._assert_execution_error(expected_msg):
# `a_typecheck_fail_test` above expects `wrapper.inner` to be a `B`.
self.scheduler.product_request(A, [Params(TypeCheckFailWrapper(A()))])
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.