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

Significantly reduce the dependencies of RuleRunner #15631

Merged
merged 2 commits into from
May 24, 2022

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented May 24, 2022

The entire init package depended on the plugins target, but the underlying reason for that dependency was to include all plugins in published distributions and PEXes... which are defined in the bin package. As a side-effect, all RuleRunner tests depended on all plugins, which significantly increased the size of their cache keys.

Before:

$ ./pants dependencies --transitive src/python/pants/testutil/rule_runner.py | wc -l
622

After:

$ ./pants dependencies --transitive src/python/pants/testutil/rule_runner.py | wc -l
105

Moral of the story: explicit deps are dangerous...?

[ci skip-rust]

@stuhood stuhood added the category:internal CI, fixes for not-yet-released features, etc. label May 24, 2022
@stuhood stuhood marked this pull request as draft May 24, 2022 20:43
@stuhood
Copy link
Member Author

stuhood commented May 24, 2022

While this could potentially be a candidate for #13393, the exact rule that we would use here isn't clear to me. It's possible that the plugins list would be better off defined in bin instead, (EDIT: Done.) and then would be declared to be private? But since it is impossible to "accidentally" depend on a target, it might not buy us a whole lot.

@stuhood stuhood force-pushed the stuhood/narrow-rule-runner-deps branch from 1477db9 to 6b30592 Compare May 24, 2022 21:41
@stuhood stuhood marked this pull request as ready for review May 24, 2022 21:41
@stuhood stuhood requested review from benjyw and thejcannon May 24, 2022 21:42
@stuhood stuhood enabled auto-merge (squash) May 24, 2022 21:51
@stuhood stuhood merged commit 1c1c0f1 into pantsbuild:main May 24, 2022
@stuhood stuhood deleted the stuhood/narrow-rule-runner-deps branch May 24, 2022 22:47
@@ -111,6 +111,7 @@ def load_build_configuration_from_source(
:raises: :class:``pants.base.exceptions.BuildConfigurationError`` if there is a problem loading
the build configuration.
"""
# NB: Backends added here must be explicit dependencies of this module.
backend_packages = FrozenOrderedSet(["pants.core", "pants.backend.project_info", *backends])
Copy link
Member

Choose a reason for hiding this comment

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

Would string import inference work here? 🤔

Copy link
Member Author

@stuhood stuhood May 25, 2022

Choose a reason for hiding this comment

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

Not out of the box I don't think... the actual underlying modules are pants.core.register and pants.backend.project_info.register.

@benjyw benjyw mentioned this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants