chore: Remove the PyRunner #4458

srilman · 2025-05-30T20:18:54Z

Changes Made

Remove the following triggers for the py-runner:

DAFT_RUNNER=py
daft.set_runner_py()
DAFT_ENABLE_NATIVE_EXECUTOR and the associated config
Also removed any remaining code and testing for the pyrunner

Checklist

Documented in API Docs (if applicable)
Documented in User Guide (if applicable)
If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)

srilman · 2025-05-30T20:42:02Z

.github/workflows/property-based-tests.yml

@@ -1,80 +0,0 @@
-name: Run property based tests with Hypothesis


Removed entirely because it only tested with the pyrunner. Lmk if it should run with the native runner instead

Could we try using the native and ray runners for this instead of removing it?

srilman · 2025-05-30T20:42:36Z

daft/context.py

@@ -94,31 +93,6 @@ def set_runner_ray(
    return DaftContext._from_native(py_ctx)


-def set_runner_py(use_thread_pool: bool | None = None, num_threads: int | None = None) -> DaftContext:


Instead of removing, would it be better to have it return an error?

Might be helpful but we also have noted that deprecation warning for a while now so it's ok to remove as well. I'll leave it to your discretion

srilman · 2025-05-30T20:42:55Z

src/common/daft-config/src/lib.rs

-            && matches!(val.trim().to_lowercase().as_str(), "1" | "true")
-        {
-            log::warn!("DAFT_ENABLE_NATIVE_EXECUTOR will be deprecated and removed in the future. Please switch to using DAFT_RUNNER=NATIVE instead.");
-            cfg.enable_native_executor = true;


Just removed the flag entirely

srilman · 2025-05-30T20:44:12Z

.github/workflows/daft-profiling.yml

@@ -70,14 +70,10 @@ jobs:
      run: python benchmarking/tpch/data_generation.py --scale_factor=${{ env.TPCH_SCALE_FACTOR }} --num_parts=${{ env.TPCH_NUM_PARTS }} --generate_parquet

    - name: Run Profiling on TPCH Benchmark
-      env:
-        DAFT_DEVELOPER_USE_THREAD_POOL: '0'


Used by pyrunner only

srilman · 2025-05-30T20:45:19Z

src/daft-context/src/lib.rs

+        "ray" => Ok(get_ray_runner_config_from_env()),
+        "py" => Err(DaftError::ValueError("The PyRunner was removed from Daft from v0.5.0 onwards. Please set the env `DAFT_RUNNER=native` instead.".to_string())),
+        _ if detect_ray_state() => Ok(get_ray_runner_config_from_env()),
+        other => Err(DaftError::ValueError(format!("Invalid runner `{other}` specified in DAFT_RUNNER env")))


Potentially breaking change: Instead of just allowing any DAFT_RUNNER=... to use native, throwing an error now. Throwing a special error for the pyrunner

kevinzwang · 2025-05-30T21:36:49Z

.github/workflows/property-based-tests.yml

@@ -1,80 +0,0 @@
-name: Run property based tests with Hypothesis


Could we try using the native and ray runners for this instead of removing it?

kevinzwang · 2025-05-30T21:37:59Z

daft/context.py

@@ -94,31 +93,6 @@ def set_runner_ray(
    return DaftContext._from_native(py_ctx)


-def set_runner_py(use_thread_pool: bool | None = None, num_threads: int | None = None) -> DaftContext:


Might be helpful but we also have noted that deprecation warning for a while now so it's ok to remove as well. I'll leave it to your discretion

kevinzwang · 2025-05-30T21:44:34Z

src/daft-context/src/lib.rs

+        "native" => Ok(RunnerConfig::Native { num_threads: None }),
+        "ray" => Ok(get_ray_runner_config_from_env()),
+        "py" => Err(DaftError::ValueError("The PyRunner was removed from Daft from v0.5.0 onwards. Please set the env to `DAFT_RUNNER=native` instead.".to_string())),
+        _ if detect_ray_state() => Ok(get_ray_runner_config_from_env()),


Remove this branch?

This would change the behavior of tests when the user doesn't specify any runner at all (either through the helper function or the env).

Maybe "" if detect_ray_state()?

I can have a warning or something though if the user specifies something other than ray but we detect ray state?

hm I would say just error

codecov · 2025-05-30T21:50:17Z

Codecov Report

Attention: Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.20%. Comparing base (25ee6fb) to head (f2bc89d).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/daft-context/src/lib.rs	76.92%	3 Missing ⚠️
src/daft-context/src/python.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4458      +/-   ##
==========================================
+ Coverage   77.49%   78.20%   +0.70%     
==========================================
  Files         850      849       -1     
  Lines      114780   114967     +187     
==========================================
+ Hits        88954    89912     +958     
+ Misses      25826    25055     -771

Files with missing lines	Coverage Δ
daft/context.py	`78.94% <ø> (-1.06%)`	⬇️
daft/runners/runner.py	`77.41% <100.00%> (ø)`
src/common/daft-config/src/lib.rs	`82.92% <ø> (+0.82%)`	⬆️
src/common/daft-config/src/python.rs	`64.68% <ø> (+0.63%)`	⬆️
src/daft-py-runners/src/lib.rs	`81.55% <ø> (+2.81%)`	⬆️
src/daft-context/src/lib.rs	`80.31% <76.92%> (+1.12%)`	⬆️
src/daft-context/src/python.rs	`68.31% <0.00%> (+1.06%)`	⬆️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Remove py runner

374277a

github-actions bot added the chore label May 30, 2025

Second removal pass

7e2a0aa

srilman commented May 30, 2025

View reviewed changes

Remove unused env

48b40f5

srilman commented May 30, 2025

View reviewed changes

srilman added 2 commits May 30, 2025 13:49

Clean up the errors a bit

09c40ca

Remove unused dep

e9d7854

srilman marked this pull request as ready for review May 30, 2025 20:54

srilman requested a review from kevinzwang May 30, 2025 20:54

srilman added 2 commits May 30, 2025 14:05

Fix tests

0f00fde

Shoot

966b1c5

kevinzwang approved these changes May 30, 2025

View reviewed changes

kevinzwang reviewed May 30, 2025

View reviewed changes

Address comment

bc93a26

srilman enabled auto-merge (squash) May 30, 2025 22:00

srilman disabled auto-merge May 30, 2025 22:00

Add it back

f2bc89d

srilman merged commit b1ffba9 into main May 31, 2025
48 of 49 checks passed

srilman deleted the slade/remove-py-runner branch May 31, 2025 00:36

		@@ -1,80 +0,0 @@
		name: Run property based tests with Hypothesis

		@@ -94,31 +93,6 @@ def set_runner_ray(
		return DaftContext._from_native(py_ctx)


		def set_runner_py(use_thread_pool: bool \| None = None, num_threads: int \| None = None) -> DaftContext:

chore: Remove the PyRunner #4458

chore: Remove the PyRunner #4458

Uh oh!

Conversation

srilman commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes Made

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

srilman commented May 30, 2025 •

edited

Loading

codecov bot commented May 30, 2025 •

edited

Loading