Print warning when defining very large remote function or actor. #2179
Conversation
|
Nice this looks decent to me; although we should be printing to stderr rather than stdout (which is a statement applicable to a larger part of the code base :) ) |
|
Yes #1884. |
|
Test PASSed. |
|
Ideally the test would be better and would actually check that the warning is printed using something like https://stackoverflow.com/questions/3892218/how-to-test-with-pythons-unittest-that-a-warning-has-been-thrown. |
|
Test FAILed. |
|
jenkins, retest this please |
|
Test FAILed. |
|
jenkins, retest this please |
|
Test FAILed. |
|
Since it's just a print string, you could do something like this: https://stackoverflow.com/questions/4219717/how-to-assert-output-with-nosetest-unittest-in-python |
There was a problem hiding this comment.
@robertnishihara do you plan on addressing the TODOs? otherwise lgtm
|
@richardliaw should be fixed now. |
|
Test PASSed. |
test/failure_test.py
Outdated
| Foo.remote() | ||
|
|
||
| # Make sure that a warning is generated. | ||
| wait_for_errors(b"pickling_large_object", 2) |
There was a problem hiding this comment.
Dumb Question: is this supposed to return an error at some point? Like what happens if this test fails? There's no unittest.assertTrue that raises a notification of failure.
There was a problem hiding this comment.
Good catch, I'll fix that.
test/failure_test.py
Outdated
| large_object | ||
|
|
||
| # Make sure that a warning is generated. | ||
| wait_for_errors(b"pickling_large_object", 1) |
There was a problem hiding this comment.
Does wait_for_errors read warnings?
There was a problem hiding this comment.
Yeah, basically, it reads everything pushed to the error_info queue
| "definition uses a large array or other object." | ||
| .format(actor_class_info["class_name"], | ||
| len(actor_class_info["class"]))) | ||
| ray.utils.push_error_to_driver( |
There was a problem hiding this comment.
What happens if I say redirect_output == True?
There was a problem hiding this comment.
It will still be printed in the background on the driver
python/ray/actor.py
Outdated
| len(actor_class_info["class"]))) | ||
| ray.utils.push_error_to_driver( | ||
| worker.redis_client, | ||
| "pickling_large_object", |
There was a problem hiding this comment.
should this be a constant somewhere?
There was a problem hiding this comment.
Yes it should be. I partially addressed this. However, this should really be done in flatbuffers.
| import ray.test.test_functions as test_functions | ||
|
|
||
| if sys.version_info >= (3, 0): | ||
| from importlib import reload | ||
|
|
||
|
|
||
| def relevant_errors(error_type): | ||
| return [info for info in ray.error_info() if info[b"type"] == error_type] |
There was a problem hiding this comment.
does the b no longer matter?
There was a problem hiding this comment.
Due to https://github.com/ray-project/ray/pull/2179/files#diff-8165ed4a37603e0f7e2eaf79e3a69bf7R1166, the error_info information consists of string instead of byte objects.
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
Print a warning if the user defines a remote function or actor whose serialized form is very large (in this case greater than 10MB). This likely indicates that the function/class definition closes over some large object.