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

Add an API to coarsen/partition Targets by their cycles #12251

Merged
merged 6 commits into from
Jul 13, 2021

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Jun 27, 2021

In order to recursively (such as for compilation) consume a graph of Targets containing cycles, we must "coarsen" the Targets in each cycle into a batch (called a strongly connected component).

This change adds a rule to compute the CoarsenedTargets for Addresses. To compile a graph of Targets with fine grained compiler invokes, you can "walk" the coarsened graph by requesting CoarsenedTargets for the roots, and then recursively requesting a compiler output per CoarsenedTarget dependency.

@patricklaw
Copy link
Member

Hey Stu, I managed to get javac working against this branch and the coarsening/cycle-detection seems to work correctly. See #12275 for details.

I'm happy to consume this as-is, although you should check out the linked PR and in particular the coarsened_components rule in javac.py. Basically, my most immediate need was not for a single-target coarsening, but rather a set of unique coarse components from an input set of targets--and I feel like this likely belongs in the generic library code rather than special-cased in javac, though it's fine to leave it there for now if you want to wait until we find another motivating use-case.

…dTargets.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
…nd rename the coarsened unit to CoarsenedTarget.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood stuhood force-pushed the stuhood/coarsened-targets branch from 1e8e0c3 to 9a927bd Compare July 9, 2021 02:32
@stuhood stuhood marked this pull request as ready for review July 9, 2021 02:38
@stuhood
Copy link
Member Author

stuhood commented Jul 9, 2021

@patricklaw : I've applied the feedback from #12275 (which changed the API: sorry). See the top commit. CoarsenedTargets is now a collection of CoarsenedTarget instances, which each have 1) a set of members of the cycle, 2) the deduped dependencies of the cycle members.

@stuhood stuhood requested review from patricklaw and removed request for benjyw July 9, 2021 02:41
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.

Cool stuff!

Comment on lines +843 to +846
fn strongly_connected_components(
py: Python,
adjacency_lists: Vec<(PyObject, Vec<PyObject>)>,
) -> CPyResult<Vec<Vec<PyObject>>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be great to do this w/ PyO3, but I see it depends on key_for which hasn't been ported yet. I can port this in a followup - it's more important we land this to unblock Patrick.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea. You can see in an earlier commit where I was attempting to do that too: the issue was that without hash/eq, I'd need to use PyDict rather than the rust native hashmap (not much of a hardship...maybe even more performant, but awkward).

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean without hash/eq?

Copy link
Member Author

Choose a reason for hiding this comment

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

PyObject itself does not have Eq/Hash implementations, so you need wrapper types around it to add those (we use Value and Key to do that).

}

Ok(
petgraph::algo::tarjan_scc(&graph)
Copy link
Contributor

Choose a reason for hiding this comment

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

I love great libraries like this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

src/python/pants/engine/target.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/graph_test.py Outdated Show resolved Hide resolved
src/python/pants/engine/internals/graph.py Outdated Show resolved Hide resolved
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood
Copy link
Member Author

stuhood commented Jul 12, 2021

I believe I've applied the feedback here: ready for another look.

@patricklaw
Copy link
Member

This looks good to me at an API level. I've merged it into my local branch and I'm working on getting javac working against the new API, though I don't expect to finish tonight. Feel free to merge as-is and I'll keep working against it on main, otherwise I should be able to prove it out in javac within a day or two.

@stuhood stuhood merged commit b0b7b0b into pantsbuild:main Jul 13, 2021
@stuhood stuhood deleted the stuhood/coarsened-targets branch July 13, 2021 15:57
@wisechengyi wisechengyi mentioned this pull request Jul 17, 2021
wisechengyi added a commit that referenced this pull request Jul 17, 2021
### Internal

* [internal] Manually fix Black lockfile to handle interpreter constraints ([#12366](#12366))
* Revert "Prefix the entire setup.py chroot. (#12359)" ([#12370](#12370))
* [internal] Cache native client binary in CI ([#12355](#12355))
* Prefix the entire setup.py chroot. ([#12359](#12359))
* [internal] Fix AWS CLI breaking due to Python 2 usage ([#12364](#12364))
* [Internal] Add `git_url()` helper to `docutil.py` ([#12352](#12352))
* [Internal] Refactor how `PythonToolBase` exposes requirements and interpreter constraints ([#12356](#12356))
* [internal] Add experimental per-tool lockfiles with Docformatter as an example ([#12346](#12346))
* Remove Pants's dpeendency on `requests` ([#12348](#12348))
* [internal] Remove `TwoStepPex` abstraction ([#12343](#12343))
* Add psycopg2-binary to default module mapping ([#12339](#12339))
* [internal] Upgrade toolchain pants plugin to 0.13.1 ([#12338](#12338))
* Add an API to coarsen/partition Targets by their cycles ([#12251](#12251))
* Prepare 2.5.1 ([#12329](#12329))
* Bootstrap fewer JVM versions in Coursier/javac tests to hopefully reduce CI flakiness ([#12325](#12325))
* Native client respects `--concurrent`. ([#12324](#12324))
* Add client lib tests. ([#12322](#12322))
* Special case enum option parse failures. ([#12281](#12281))
patricklaw added a commit that referenced this pull request Aug 21, 2021
…tion, including cycles created by subtargets. (#12231)

Consume CoarsenedTargets (added in #12251) in javac for cycle resolution, including cycles created by subtargets.
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.

3 participants