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

Allow reporters to see the correct end_time for a workunit #7389

Merged
merged 3 commits into from Mar 19, 2019

Conversation

Projects
None yet
4 participants
@codealchemy
Copy link
Contributor

commented Mar 15, 2019

Problem

The end_time for a workunit is not accessible to reports, since it's set after we tell the report that the workunit is complete.

Solution

Set the end_time on the workunit before notifying reporter's that it's complete.

Result

Reporters can now access an accurate end_time for workunits. This will be used for adjacent work (in flight) that returns a more complete representation of the build than is currently returned by the RunTracker/reporters.

@Eric-Arellano
Copy link
Contributor

left a comment

Cool fix!

Show resolved Hide resolved src/python/pants/base/workunit.py Outdated
@baroquebobcat
Copy link
Contributor

left a comment

It'd be nice if this included a test that exercised it.

path, duration, self_time, is_tool = workunit.end()
self.report.end_workunit(workunit)
workunit.close_outputs()

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 15, 2019

Contributor

I'm a little confused as to why we're separating out the closing of outputs from the workunit end. Could you explain that bit? I feel like it makes more sense for workunit.end() to handle that, but if it's preventing something from happening it'd be good to better understand that.

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 15, 2019

Author Contributor

This is so we don't close outputs that the report / reporters might attempt to read - such as in report.end_workunit(workunit) called on l496, which (among other things) calls _notify():

for label, output in list(workunit.outputs().items()):
s = output.read().decode('utf-8')
if len(s) > 0:
for reporter in self._reporters.values():
reporter.handle_output(workunit, label, s)

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 15, 2019

Contributor

I see. So, does that mean that there'd be test failures without splitting end/close_outputs?

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 15, 2019

Author Contributor

Yup (ValueError - seek of closed file)

codealchemy added some commits Mar 14, 2019

@codealchemy codealchemy force-pushed the codealchemy:fix-end-time-for-report branch from 400d676 to 31d2213 Mar 18, 2019

@benjyw

benjyw approved these changes Mar 19, 2019

@benjyw benjyw merged commit 136f9cf into pantsbuild:master Mar 19, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@codealchemy codealchemy deleted the codealchemy:fix-end-time-for-report branch Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.