search

pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.3k stars 634 forks source link

Stop consulting TransitiveTargets for `ChosenResolve` #16592

Open Eric-Arellano opened 2 years ago

Eric-Arellano commented 2 years ago

This rule is inefficient because it consults TransitiveTargets solely to validate deps are compatible with the resolve from the roots

https://github.com/pantsbuild/pants/blob/4ef5b6a5b3a6b304e7a780c63fe62edc4cd3c154/src/python/pants/backend/python/util_rules/pex_from_targets.py#L219-275

We can instead use dependency validation to make sure that all deps share the same resolve:

https://github.com/pantsbuild/pants/blob/4ef5b6a5b3a6b304e7a780c63fe62edc4cd3c154/src/python/pants/backend/python/target_types_rules.py#L518-L576

Then, our ChosenResolve rule only needs to consult the roots, which is much faster.

Eric-Arellano commented 2 years ago

I believe that this would also allow us to improve the performance of partition._by_interpreter_constraints_and_resolve?

https://github.com/pantsbuild/pants/blob/4ef5b6a5b3a6b304e7a780c63fe62edc4cd3c154/src/python/pants/backend/python/util_rules/partition.py#L21-L60

stuhood commented 2 years ago

Then, our ChosenResolve rule only needs to consult the roots, which is much faster.

Yea, makes sense.

I believe that this would also allow us to improve the performance of partition._by_interpreter_constraints_and_resolve?

I don't think so... it already takes the literal resolve value of the "representative" target in a CoarsenedTarget... essentially, it looks at the root resolve.

But that's because it is relying on a consumer later validating the matching resolve... which would be where ChosenResolve comes into play.

Eric-Arellano commented 2 years ago

believe that this would also allow us to improve the performance of partition._by_interpreter_constraints_and_resolve?

I don't understand CoarsenedTargets well -- isn't it essentially doing a more efficient TransitiveTargets? That is, it expands the dependencies? I'm suggesting we solely need to look at the roots.

stuhood commented 2 years ago

believe that this would also allow us to improve the performance of partition._by_interpreter_constraints_and_resolve?

I don't understand CoarsenedTargets well -- isn't it essentially doing a more efficient TransitiveTargets? That is, it expands the dependencies? I'm suggesting we solely need to look at the roots.

It is computing transitive dependencies, but exactly once across many roots, rather than once per root.

All of the consumers of that function do actually need the transitive dependencies, so someone will have to compute them eventually.