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

[BUG] Check env in benchmarking script #3297

Merged
merged 3 commits into from
Nov 14, 2024
Merged

Conversation

colin-ho
Copy link
Contributor

@colin-ho colin-ho commented Nov 14, 2024

Using ctx.get_or_create_runner in benchmarking warmup code / metrics builder causes subsequent ray.inits to crash. Just check the DAFT_RUNNER environment var instead, which should be set.

Tested:

@github-actions github-actions bot added the bug Something isn't working label Nov 14, 2024
Copy link

codspeed-hq bot commented Nov 14, 2024

CodSpeed Performance Report

Merging #3297 will degrade performances by 47.65%

Comparing colin/fix-benchmarking-script (1d52ee2) with main (711e862)

Summary

❌ 2 regressions
✅ 15 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main colin/fix-benchmarking-script Change
test_iter_rows_first_row[100 Small Files] 269 ms 373.6 ms -28%
test_show[100 Small Files] 22.3 ms 42.5 ms -47.65%

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.50%. Comparing base (711e862) to head (1d52ee2).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3297      +/-   ##
==========================================
- Coverage   77.50%   77.50%   -0.01%     
==========================================
  Files         666      666              
  Lines       81335    81333       -2     
==========================================
- Hits        63041    63036       -5     
- Misses      18294    18297       +3     

see 3 files with indirect coverage changes

@@ -212,7 +222,7 @@ def warmup_environment(requirements: str | None, parquet_folder: str):
"""Performs necessary setup of Daft on the current benchmarking environment"""
ctx = daft.context.get_context()

if ctx.get_or_create_runner().name == "ray":
if get_daft_benchmark_runner_name() == "ray":
runtime_env = get_ray_runtime_env(requirements)

ray.init(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ray runner gets initialized in get_or_create_runner(), which calls ray.init, so the subsequent ray.init errors.

@colin-ho colin-ho requested a review from jaychia November 14, 2024 14:44
@jaychia jaychia merged commit 25c3b26 into main Nov 14, 2024
49 of 50 checks passed
@jaychia jaychia deleted the colin/fix-benchmarking-script branch November 14, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants