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] Propagate URL download expressions max_connections to S3Config #1708

Merged
merged 2 commits into from
Dec 8, 2023

Conversation

jaychia
Copy link
Contributor

@jaychia jaychia commented Dec 8, 2023

Problem: URL downloads had a regression when the default value on S3Config changed from 64 to 8.

This is because our URL download's max_connections parameter acts as a "throttling" mechanism for the maximum number of tasks in-flight from a channel buffer.

However, if this number is higher than the S3 client's max_connections, then it has no effect since the total number of connections to S3 is capped at S3Config.max_connections * num_threads.

In this PR, we just override the S3Config's value with the one that is passed into the expression.

@github-actions github-actions bot added the bug Something isn't working label Dec 8, 2023
@jaychia jaychia enabled auto-merge (squash) December 8, 2023 06:19
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #1708 (4212d25) into main (a53cd51) will decrease coverage by 0.23%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1708      +/-   ##
==========================================
- Coverage   85.11%   84.88%   -0.23%     
==========================================
  Files          55       55              
  Lines        5368     5393      +25     
==========================================
+ Hits         4569     4578       +9     
- Misses        799      815      +16     
Files Coverage Δ
daft/expressions/expressions.py 92.09% <100.00%> (+0.04%) ⬆️

... and 10 files with indirect coverage changes

@jaychia jaychia merged commit 9bf06cb into main Dec 8, 2023
40 checks passed
@jaychia jaychia deleted the jay/default-url-download-connections branch December 8, 2023 06:29
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.

1 participant