-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Revamped implementations of remote print()
and warn()
, fixing #7095
#7129
Conversation
…ede() and rejoin(), which are also meant to be called from code running on workers.
Can one of the admins verify this patch? Admins can comment |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 6h 32m 26s ⏱️ - 8m 50s For more details on these failures, see this check. Results for commit 96a8df8. ± Comparison against base commit 07e2259. ♻️ This comment has been updated with latest results. |
(All tests pass; the linting errors seem to be a pre-existing problem, not from my code.) |
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 PR rewrites the remote
print()
andwarn()
functions to be more correct and better documented, as discussed in #7095, and extending the initial work by @fjetter in #5217. Some notes about the implementation:print()
relies on msgpack to distinguish correctly between string-valued and None-valued arguments. I've special-cased thefile
kwarg to disallow using this function to write to arbitrary file-like objects (because it wouldn't be clear how that should work in the possibly remote client thread), while still allowing a distinction between standard out and standard error.warn()
handles its own serialization of themessage
andcategory
arguments into bytes because, if we want it to be truly drop-in, these might be Warning instance objects or classes, which msgpack won't serialize out of the box. See the test function for the various use cases. I feel like the only way this breaks down is if the user does something really bizarre like emit a custom Warning subclass that stores a ton of data in itself.warn()
ignores thestacklevel
andsource
arguments in the remote client, because the former would be meaningless in the client's thread and the latter would entail serializing an arbitrary object, which I'd rather avoid.I've also stuck these functions into the API docs. Assuming this is merged, a good follow-up PR would be to introduce them in the "Debug" page of the main dask docs.