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 default logs method to Spec Cluster #2889
Conversation
This gathers logs through the scheduler using existing methods, and then returns them as nicely rendered summary/details outputs.
distributed/deploy/spec.py
Outdated
A dictionary of logs, with one item for the scheduler and one for | ||
each worker | ||
""" | ||
return self.sync(self._logs, workers=workers, nanny=nanny) |
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.
cc @jacobtomlinson you may want to briefly skim through this, just to get a sense of how the combination async/sync API stuff tends to work within dask-distributed today.
Also @jcrist are you comfortable with this API for dask-yarn? |
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.
Overall this seems fine. What about getting the scheduler logs, for implementations with a remote scheduler?
distributed/deploy/spec.py
Outdated
A list of worker addresses to select. | ||
Defaults to all workers | ||
nanny: bool, optional | ||
Get the logs from the nannies rather than the workers |
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.
For implementations that grab logs from the backing system (e.g. kubernetes) this option may not make any sense - the only logs will be stdout/stderr of the process tree (IIRC the nanny and the worker share stdout/stderr). I'd be tempted to remove this option entirely.
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.
Done
Currently the scheduler is included in these logs. It's the first entry with the name "Scheduler" (see notebook link above for an example)" |
Perhaps there should be a |
Sure. Done. The tests that were just added might be a nice way to check this |
Thanks for the engagement Jim! |
Builds off of #2875
cc @jacobtomlinson