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

Fix memoization of CoarsenedTarget.closure #17516

Merged
merged 1 commit into from Nov 9, 2022

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Nov 9, 2022

Memoization of CoarsenedTargets.closure() relies on shared memoization across a walk in each CoarsenedTarget. That memoization was not working.

Fixes #17509.

@stuhood stuhood added needs-cherrypick category:bugfix Bug fixes for released features labels Nov 9, 2022
@stuhood stuhood added this to the 2.15.x milestone Nov 9, 2022
@stuhood stuhood enabled auto-merge (squash) November 9, 2022 19:27

ct1 = ct([a], [])
ct2 = ct([b, c], [ct1])
ct3 = ct([d, e], [ct1, ct2])
Copy link
Contributor

@danxmoran danxmoran Nov 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't fully grokked the invariants of CTs yet: should we test for overlapping members between CTs as well? Or is that not a situation we can get into?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No: that should be impossible due to how CTs are constructed. And that bit has existing tests:

def test_coarsened_targets(transitive_targets_rule_runner: RuleRunner) -> None:
"""CoarsenedTargets should "coarsen" a cycle into a single CoarsenedTarget instance."""
transitive_targets_rule_runner.write_files(
{
"dep.txt": "",
"t1.txt": "",
"t2.txt": "",
# Cycles are only tolerated for file-level targets like `python_source`.
# TODO(#12871): Stop relying on only generated targets having cycle tolerance.
"BUILD": dedent(
"""\
generator(name='dep', sources=['dep.txt'])
generator(name='t1', sources=['t1.txt'], dependencies=['dep.txt:dep', 't2.txt:t2'])
generator(name='t2', sources=['t2.txt'], dependencies=['t1.txt:t1'])
"""
),
}
)
def assert_coarsened(
a: Address, expected_members: List[Address], expected_dependencies: List[Address]
) -> None:
coarsened_targets = transitive_targets_rule_runner.request(
CoarsenedTargets,
[Addresses([a])],
)
assert sorted(t.address for t in coarsened_targets[0].members) == expected_members
# NB: Only the direct dependencies are compared.
assert (
sorted(d.address for ct in coarsened_targets[0].dependencies for d in ct.members)
== expected_dependencies
)
# Non-file-level targets are already validated to not have cycles, so they coarsen to
# themselves.
assert_coarsened(
Address("", target_name="dep"),
[Address("", target_name="dep")],
[Address("", relative_file_path="dep.txt", target_name="dep")],
)
assert_coarsened(
Address("", target_name="t1"),
[Address("", target_name="t1")],
[
Address("", relative_file_path="t1.txt", target_name="t1"),
Address("", relative_file_path="t2.txt", target_name="t2"),
],
)
assert_coarsened(
Address("", target_name="t2"),
[Address("", target_name="t2")],
[
Address("", relative_file_path="t1.txt", target_name="t1"),
Address("", relative_file_path="t2.txt", target_name="t2"),
],
)
# File-level targets not involved in cycles coarsen to themselves.
assert_coarsened(
Address("", relative_file_path="dep.txt", target_name="dep"),
[Address("", relative_file_path="dep.txt", target_name="dep")],
[],
)
# File-level targets involved in cycles will coarsen to the cycle, and have only dependencies
# outside of the cycle.
cycle_files = [
Address("", relative_file_path="t1.txt", target_name="t1"),
Address("", relative_file_path="t2.txt", target_name="t2"),
]
assert_coarsened(
Address("", relative_file_path="t1.txt", target_name="t1"),
cycle_files,
[Address("", relative_file_path="dep.txt", target_name="dep")],
)
assert_coarsened(
Address("", relative_file_path="t2.txt", target_name="t2"),
cycle_files,
[Address("", relative_file_path="dep.txt", target_name="dep")],
)

Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed it's working in practice via pylint on Color's repo

@stuhood stuhood merged commit c1514b7 into pantsbuild:main Nov 9, 2022
@stuhood stuhood deleted the stuhood/ct-closure-fix branch November 9, 2022 20:21
stuhood added a commit to stuhood/pants that referenced this pull request Nov 9, 2022
Memoization of `CoarsenedTargets.closure()` relies on shared memoization across a walk in each `CoarsenedTarget`. That memoization was not working.

Fixes pantsbuild#17509.
stuhood added a commit that referenced this pull request Nov 9, 2022
…17518)

Memoization of `CoarsenedTargets.closure()` relies on shared memoization across a walk in each `CoarsenedTarget`. That memoization was not working.

Fixes #17509.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CoarsenedTargets.closure() returns duplicate targets
3 participants