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

ci: Remove daft tracing #3692

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

ci: Remove daft tracing #3692

wants to merge 2 commits into from

Conversation

raunakab
Copy link
Contributor

Overview

Daft tracing was enabled by default. However, this can negatively affect performance, so this PR opts to make daft-tracing disabled by default.

However, end-users can still choose to turn on daft-tracing by default (should they so wish) by adding the DAFT_ENABLE_RAY_TRACING=1 environment variable.

@github-actions github-actions bot added the ci label Jan 16, 2025
@raunakab raunakab marked this pull request as ready for review January 16, 2025 00:06
Copy link

codspeed-hq bot commented Jan 16, 2025

CodSpeed Performance Report

Merging #3692 will degrade performances by 32.34%

Comparing remove-daft-tracing (73ca820) with main (34d2036)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 25 untouched benchmarks

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

Benchmarks breakdown

Benchmark main remove-daft-tracing Change
test_iter_rows_first_row[100 Small Files] 197.1 ms 165.9 ms +18.77%
test_show[100 Small Files] 16.1 ms 23.8 ms -32.34%

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.79%. Comparing base (34d2036) to head (73ca820).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3692   +/-   ##
=======================================
  Coverage   77.79%   77.79%           
=======================================
  Files         729      729           
  Lines       90477    90468    -9     
=======================================
- Hits        70384    70381    -3     
+ Misses      20093    20087    -6     

see 3 files with indirect coverage changes

@raunakab
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants