Consodiate CI Python tests and fix bug about multiple ray.init#4195
Consodiate CI Python tests and fix bug about multiple ray.init#4195robertnishihara merged 7 commits intoray-project:masterfrom
Conversation
|
cc @williamma12 |
|
Test FAILed. |
python/ray/remote_function.py
Outdated
| worker = ray.worker.get_global_worker() | ||
| worker.check_connected() | ||
|
|
||
| if self._last_export_time < worker._last_shutdown_time: |
There was a problem hiding this comment.
This seems like a nice way to allow remote functions to be reused between multiple sessions without redefining them.
There are some moderately rare ways this could go wrong since time.time() is not monotone.
That could be fixed by using some sort of counter. E.g., the worker keeps some sort of session_index which increments every time you call shutdown. Then when a remote function is exported, we set its session_index equal to the worker's session_index.
I haven't thought through the details, but I'm not sure if this will work for nested remote functions.
python/ray/remote_function.py
Outdated
| worker = ray.worker.get_global_worker() | ||
| worker.check_connected() | ||
|
|
||
| if self._last_export_time < worker._last_shutdown_time: |
There was a problem hiding this comment.
Also, was this fix needed to get some test working? If so, which one?
There was a problem hiding this comment.
Yeah, lots of tests will fail without this fix. Because when we pass in the test directory, pytest will first import all files. Then all the remote functions will only be exported to the first session. Also see #4192
There was a problem hiding this comment.
Looks good, just a minor issue with comments! I am curious as to how the tests passed
EDIT: Nevermind I followed the issues back to #3897, which was why some of the tests were failing. Nice catch!
| - python -m pytest -v --durations=10 python/ray/tests/test_credis.py | ||
| - python -m pytest -v --durations=10 python/ray/tests/test_node_manager.py | ||
| - python -m pytest -v --durations=10 python/ray/tests/test_signal.py | ||
| # TODO(yuhguo): object_manager_test.py requires a lot of CPU/memory, and |
There was a problem hiding this comment.
Let's try to keep this comment in so we dont forget to move it over to jenkins eventually
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
| - python -m pytest -v --durations=10 python/ray/tune/test/ray_trial_executor_test.py | ||
| - python -m pytest -v --durations=10 python/ray/tune/test/automl_searcher_test.py | ||
| # `cluster_tests.py` runs on Jenkins, not Travis. | ||
| - python -m pytest -v --durations=30 --ignore=python/ray/tune/cluster_tests.py python/ray/tune/test |
There was a problem hiding this comment.
I'm slightly surprised that this works because I thought pytest only picks up tests that have test_ as the start of the name. But it seems to be working..
What do these changes do?
Use the test directory as the parameter of pytest, instead of listing test files one by one. So we don't need to modify here when we add or remove test files. Also, it'd be easier to find the globally slowest test cases.
Also fixes #4192.
Related issue number
Closes #4192.