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.

Sign up for GitHub

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

Jump to bottom

Shortcircuit local distribution source subsetting if there are no dists. #14564

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Shortcircuit local distribution source subsetting if there are no dists. #14564

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/python/pants/backend/python/util_rules/local_dists.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,12 @@ async def build_local_dists(
),
)

if not wheels:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also shortcircuit at line 146? Other than an extra 2 lines of code, I see no reason not to.

Also I'm not sure this informative comment is necessary - short circuiting usually seems like the right thing to do!

Copy link
Member Author

@stuhood stuhood Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should we also shortcircuit at line 146? Other than an extra 2 lines of code, I see no reason not to.

That would require adjusting the signature of LocalDistsPex to make the pex optional, which affects a lot of callsites. I started down that path, but shortcircuiting here instead allowed for the null-object pattern.

Also I'm not sure this informative comment is necessary - short circuiting usually seems like the right thing to do!

Yeaaaa. @benjyw and I both felt that landing this was punting on determining what the actual performance issue was in the trailing code: he's following up on our best guess there. If he determines why then he can delete the comment perhaps, but in the meantime let's leave it.

# The source calculations below are not (always) cheap, so we skip them if no wheels were
# produced. See https://github.com/pantsbuild/pants/issues/14561 for one possible approach
# to sharing the cost of these calculations.
return LocalDistsPex(dists_pex, request.sources)

# We check source roots in reverse lexicographic order,
# so we'll find the innermost root that matches.
source_roots = sorted(request.sources.source_roots, reverse=True)
Expand Down