Skip to content
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

[FEAT] Automatically use Ray Runner if Ray is initialized #2282

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jaychia
Copy link
Contributor

@jaychia jaychia commented May 17, 2024

  1. Automatically switches Daft to use the Ray Runner if a user calls ray.init(...) before running any Daft querying
  2. Also switches behavior to try and deprecate the DAFT_RAY_ADDRESS environment variable so that we can centralize on the normal RAY_ADDRESS behavior

This PR ensures the following behavior:

  • If a user explicitly calls daft.context.set_runner_ray/py, this overrides all behavior
    • If a user calls daft.context.set_runner_ray with a specified address, but Ray is already initialized, we warn them that their address is being ignored
  • Otherwise, on first execution Daft will attempt to retrieve the runner config from the current environment:
    • Check for the DAFT_RUNNER environment variable for RAY/PY
    • Check to see if Ray is initialized, and if we aren't running in a Ray worker: RAY
    • Fallback onto: PY

Ray connection detection, on driver vs on worker:

image

Warning if set_runner_ray is called with an address after Ray is already initialized:
image

@github-actions github-actions bot added enhancement New feature or request documentation Improvements or additions to documentation labels May 17, 2024
Copy link
Contributor

@kevinzwang kevinzwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a few comments, but otherwise looks great!

A few more thoughts:

  • The usage of the lock seems correct to me 👍
  • Have you tested this manually, or maybe could we write some basic tests for this behavior?
  • In set_runner_py, the docstrings say "this is the default behavior", that should be changed.

daft/context.py Outdated Show resolved Hide resolved
daft/runners/ray_runner.py Show resolved Hide resolved
Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes are missing coverage. Please review.

Please upload report for BASE (main@e836290). Learn more about missing BASE report.

Current head e7ab3d2 differs from pull request most recent head da4e0de

Please upload reports for the commit da4e0de to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2282   +/-   ##
=======================================
  Coverage        ?   79.12%           
=======================================
  Files           ?      471           
  Lines           ?    54404           
  Branches        ?        0           
=======================================
  Hits            ?    43049           
  Misses          ?    11355           
  Partials        ?        0           
Files Coverage Δ
daft/runners/ray_runner.py 90.35% <50.00%> (ø)
daft/context.py 76.29% <70.83%> (ø)

@kevinzwang kevinzwang self-requested a review May 30, 2024 00:10
Copy link
Contributor

@kevinzwang kevinzwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants