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

Record critical path timings of goals #5609

Merged
merged 3 commits into from Mar 20, 2018

Conversation

Projects
None yet
4 participants
@illicitonion
Copy link
Contributor

illicitonion commented Mar 16, 2018

This is the sum of the cumulative timing of each goal, and the goals
that goal transitively depends on.

Record critical path timings of goals
This is the sum of the cumulative timing of each goal, and the goals
that goal transitively depends on.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/criticalpath branch from 5921c77 to 958e179 Mar 16, 2018

@@ -250,6 +250,10 @@ def _execute_engine(self):
return 1

engine = RoundEngine()

sorted_goal_infos = engine.sort_goals(self._context, self._goals)

This comment has been minimized.

@illicitonion

illicitonion Mar 16, 2018

Contributor

This means we actually do this sorting twice, because execute is a public API which means I can't change it to pass in the sorted_goal_infos instead of self._goals. I guess I could memoize it in the engine... But this also feels like an API which maybe shouldn't actually be public, because people probably shouldn't be poking the engine directly from plugins?

This comment has been minimized.

@wisechengyi

wisechengyi Mar 17, 2018

Contributor

Are you referring to

def execute(self, context, goals):
"""Executes the supplied goals and their dependencies against the given context.
:param context: The pants run context.
:param list goals: A list of ``Goal`` objects representing the command line goals explicitly
requested.
:returns int: An exit code of 0 upon success and non-zero otherwise.
"""
? If so, I don't see why it cannot take a list of sorted goals. Last changes to it were 594389c and c8c77b7 which are > 3 years away.

This comment has been minimized.

@illicitonion

illicitonion Mar 19, 2018

Contributor

It would be changing goals from being a List<Goal> to a List<GoalInfo> which feels like a non-trivial API change... (Particularly given GoalInfo is currently not even a public type)

@stuhood
Copy link
Member

stuhood left a comment

Thanks Daniel!


timings = AggregatedTimings()
for goal, deps in transitive_dependencies.items():
label = "main:{}".format(goal)

This comment has been minimized.

@stuhood

stuhood Mar 18, 2018

Member

Perhaps overkill, but: there is a constant for the DEFAULT_ROOT_NAME that you could use here.

illicitonion added some commits Mar 19, 2018

Only propose alternative target roots in engine execution
Don't also do so in the GoalRunner, which doesn't do proper error handling
@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Mar 20, 2018

@stuhood Please take another look, the last commit is something I'd like another pair of eyes over...

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Mar 20, 2018

Looks fine, thanks. Presumably: "because side-effects"?

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Mar 20, 2018

Yeah, a combination of "side effects" and "can error, but doesn't have error handling wrappers in the context where I'm now calling it" :)

@illicitonion illicitonion merged commit c982ba2 into pantsbuild:master Mar 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ity
Copy link
Member

ity left a comment

thanks for doing this - lgtm!

@illicitonion illicitonion deleted the twitter:dwagnerhall/criticalpath branch Apr 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment