-
-
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
Revert refactor to utils.Log[s]
and Cluster.get_logs
#4941
Conversation
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.
Thanks @charlesbluca! To make sure we're informed when any user-facing structure changes for Cluster.get_logs()
, can we add/update a few small tests to make sure:
Cluster.get_logs()
returns adict
ofstr
s (probably updatingdistributed/deploy/tests/test_spec_cluster.py::test_logs
)Log
is astr
subclass (probably updatingdistributed/tests/test_utils.py::test_logs
)Logs
is adict
subclass (probably updatingdistributed/tests/test_utils.py::test_logs
)
To be clear, we can change the structure of our output logs in the future, but we should raise deprecation or future warnings to let users know about any changes ahead of time |
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.
Thanks for the quick fix @charlesbluca, this looks good to me.
We certainly can guard against interface changes with unit tests as @jrbourbeau suggests, but ultimately I'd like to see a stronger delineation between the public and private dask APIs, especially for things that are intended to be subclassed. That way it would be clearer which things are more/less dangerous to change. That's well outside the scope here though :)
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.
Thanks for such a quick fix @charlesbluca! Will merge after CI finishes
Thanks @charlesbluca ! |
The refactor of
utils.Log[s]
andCluster.get_logs
in #4909 can lead to errors in downstream projects that use these functions. This PR restores the original behavior of these functions, while making some minor adjustments to achieve what was done in #4909 (styling conditional on log level, tighter spacing between log entries).black distributed
/flake8 distributed
/isort distributed