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

Implement cycle detection in transitive_targets, and tolerate cycles in file-addresses. #10409

Merged
merged 3 commits into from
Jul 22, 2020

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Jul 21, 2020

Problem

After more deeply investigating the issues with #10230 (demonstrated in #10393 and its revert), it doesn't seem like the right idea to rely on the engine's cycle detection (which the implementation of #10230 showed should primarily be used for deadlock detection) to expose high level cycle tolerance for the build graph.

Solution

Move to iterative transitive target expansion (à la #10046), and cycle detect afterward using DFS with a stack of visited entries.

Result

Fixes #10059, and closes #10229. A followup will back out portions of #10230 (but not all of it, as there were a number of other improvements mixed in).

[ci skip-rust-tests]

@coveralls
Copy link

coveralls commented Jul 21, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 43aa188 on stuhood:stuhood/high-level-cycle-tolerance into c827416 on pantsbuild:master.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you, Stu!

Only not yet approving because of the possible performance implications - code looks great otherwise.

Address("", target_name="t2.txt", generated_base_target_name="t2"),
]

def do_test_failed_cycle(self, address_str: str, cyclic_address_str: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: consider renaming to assert_failed_cycle() for alignment with Pytest and unittest.


return TransitiveTargets(tuple(tt.root for tt in tts), FrozenOrderedSet(dependencies))
transitive_targets = TransitiveTargets(tuple(targets), FrozenOrderedSet(visited))
_detect_cycles(tuple(t.address for t in targets), dependency_mapping)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect running DFS has a real performance impact on mission-critical code. But I also don't know of another way to detect these cycles at the same time as we're resolvings deps via BFS..

Any idea how bad the impact is when running ./pants dependencies --transitive ::?

One thing Benjy and I started to discuss this morning is whether to have a global flag --tolerate-cycles. Gordon gave feedback that he wouldn't want to tolerate cycles with dep inference / generated subtargets either. We would only run this code if you have --no-tolerate-cycles.

Copy link
Member Author

@stuhood stuhood Jul 21, 2020

Choose a reason for hiding this comment

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

It appears to add about 4.8% to the runtime.

One thing Benjy and I started to discuss this morning is whether to have a global flag --tolerate-cycles.

I'd prefer something that could be toggled on a target by target basis, but failing that, maybe a ternary of --tolerate-cycles=file|target|none.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would be fine with me to be a target by target basis. I'd rather have that than always no matter what tolerating generated subtargets.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as there's a way to opt-in to (or out of) enforcing cycle hygiene.

@stuhood stuhood force-pushed the stuhood/high-level-cycle-tolerance branch from e5d4502 to 01f7336 Compare July 21, 2020 03:52
Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

I would love to see a flag that allows you to to determine whether existence of a cycle should be A) an exception, B) a warning, C) silent.

Re performance, this would be cached as long as BUILD files don't change, right?

@stuhood
Copy link
Member Author

stuhood commented Jul 21, 2020

Re performance, this would be cached as long as BUILD files don't change, right?

Yes.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks! I wish there was a way to combine the cycle detection with the hydration for the sake of performance, but I don't know of any such way.

src/python/pants/engine/internals/graph.py Show resolved Hide resolved
src/python/pants/engine/internals/graph.py Show resolved Hide resolved
src/python/pants/engine/internals/graph.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/graph.py Show resolved Hide resolved
src/python/pants/engine/internals/graph_test.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/graph.py Show resolved Hide resolved
src/python/pants/engine/internals/graph.py Outdated Show resolved Hide resolved
[ci skip-rust-tests]
[ci skip-rust-tests]
@stuhood stuhood force-pushed the stuhood/high-level-cycle-tolerance branch from 01f7336 to 43aa188 Compare July 22, 2020 03:15
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you!

@stuhood stuhood merged commit 8105c6b into pantsbuild:master Jul 22, 2020
@stuhood stuhood deleted the stuhood/high-level-cycle-tolerance branch July 22, 2020 04:24
stuhood added a commit that referenced this pull request Sep 17, 2020
### Problem

"weak" `Get`s were a potential solution for optionally allowing for cycles in the runtime `Graph`, but they were only partially implemented (see #10229), and not particularly trustworthy. We ended up going with a higher level solution in #10409, but the weak-`Get`s code still exists, and increases the complexity of the `graph` crate unnecessarily.

### Solution

Revert #10230. Closes #10229 and closes #10394.

[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow a strong-weak cycle to be entered from either end Tolerate build graph cycles
4 participants