-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Add scheduler log tab to performance reports #4909
Conversation
This looks good to me. cc @jacobtomlinson |
we could add timestamps to the get_logs e.g. def get_logs(self, comm=None, n=None, timestamps=False):
deque_handler = self._deque_handler
if n is None:
L = list(deque_handler.deque)
else:
L = deque_handler.deque
L = [L[-i] for i in range(min(n, len(L)))]
if timstamps:
return [(msg.created, msg.levelname, deque_handler.format(msg)) for msg in L]
else:
return [(msg.levelname, deque_handler.format(msg)) for msg in L] with the toggle this is fully backwards compat and the report could filter This can be obviously done in another PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great.
I just want to draw your attention to the existing Log
and Logs
objects which we use to give HTML reprs to get_logs
calls today.
distributed/distributed/utils.py
Lines 1272 to 1292 in b707c29
class Log(str): | |
"""A container for logs""" | |
def _repr_html_(self): | |
return "<pre><code>\n{log}\n</code></pre>".format( | |
log=html.escape(self.rstrip()) | |
) | |
class Logs(dict): | |
"""A container for multiple logs""" | |
def _repr_html_(self): | |
summaries = [ | |
"<details>\n" | |
"<summary style='display:list-item'>{title}</summary>\n" | |
"{log}\n" | |
"</details>".format(title=title, log=log._repr_html_()) | |
for title, log in sorted(self.items()) | |
] | |
return "\n".join(summaries) |
I wonder if these can be ehnanced/reused here?
Would you mind also adding a test or an assertion line to the performance_report test ? Thanks for picking up this issue @charlesbluca ! |
Sure, thanks for catching that @quasiben 🙂 Thanks for the tip @fjetter! Timestamps could also be useful as part of the log output itself I'd imagine - would definitely be happy to open up a follow up PR implementing that. I like the idea of reusing the HTML reprs here, I think a potential refactor to match the behavior of this Jinja template would be to change these objects such that:
|
Flaky test, rerunning. |
Are we good to merge here? |
Think so, the test failures seem to be flaky - @quasiben is the added assertion in the perf report test sufficient? |
Adds a tab to the performance report with the logs of the scheduler. Note that these are all the logs currently contained in the scheduler's log deque. and not just the logs generated in the
performance_report
context:black distributed
/flake8 distributed
/isort distributed