feat: make ray optional #878
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
evadb/executor/exchange_executor.py
Outdated
| @@ -65,6 +63,9 @@ def build_inner_executor(self, inner_executor): | |||
| self.inner_executor.children = [QueueReaderExecutor()] | |||
|
|
|||
| def exec(self) -> Iterator[Batch]: | |||
| print("RAY") | |||
evadb/executor/ray_utils.py
Outdated
| @@ -26,8 +22,10 @@ class StageCompleteSignal: | |||
| pass | |||
|
|
|||
|
|
|||
| @ray.remote(num_cpus=0) | |||
| def ray_wait_and_alert(tasks: List[ray.ObjectRef], queue: Queue): | |||
| def ray_wait_and_alert(tasks, queue): | |||
There was a problem hiding this comment.
Did you try running Ray enabled? I think this won't work if you remove the decorator.
Is this a good solution? https://stackoverflow.com/questions/16192448/conditional-definition-of-a-function-in-a-module
There was a problem hiding this comment.
Yes, it worked fine even without the decorators.
There was a problem hiding this comment.
https://docs.ray.io/en/latest/ray-core/api/doc/ray.remote.html
This seems like a simpler solution: f_with_2_gpus = f.options(num_gpus=2)
There was a problem hiding this comment.
I think if we specify num_gpus on the Ray side, this (#817) issue may occur.
| @@ -25,7 +25,7 @@ server: | |||
| socket_timeout: 60 | |||
|
|
|||
| experimental: | |||
| ray: True | |||
| ray: False | |||
There was a problem hiding this comment.
We need to update the circle ci test setup, after making the default to be False. Currently we have all tests with ray enabled except COVERAGE. If we want to keep this test setup, we need to modify the circleci configuration accordingly.
There was a problem hiding this comment.
Yes, by default, let us disable it. For each python version, we could have build with ray disabled and ray enabled (ray installed and configuration variable set to True).
| @@ -115,7 +116,8 @@ def __init__(self, config: ConfigurationManager): | |||
| LogicalVectorIndexScanToPhysical(), | |||
| ] | |||
|
|
|||
| ray_enabled = config.get_value("experimental", "ray") | |||
| # ray_enabled = config.get_value("experimental", "ray") | |||
| ray_enabled = is_ray_available() | |||
There was a problem hiding this comment.
We can not turn off ray when ray is installed. Shall we do config.get_value("experimental", "ray") and is_ray_available() instead?
There was a problem hiding this comment.
@gaurav274 We could probably re-enable this option. In addition to ray being installed, we can let user update this config variable to actually use ray. That will further reduce the likelihood of ray being accidentally used.
👋 Thanks for submitting a Pull Request to EvaDB!
🙌 We want to make contributing to EvaDB as easy and transparent as possible. Here are a few tips to get you started:
👉 Please see our ✅ Contributing Guide for more details.