fix: increase streaming chunk size for CSV and Excel view downloads - #1817
fix: increase streaming chunk size for CSV and Excel view downloads#1817jacalata wants to merge 3 commits into
Conversation
|
Should this use the |
Raises iter_content chunk size from 1024 to 65536 bytes in _get_view_csv and _get_view_excel. The 1-KB default required ~200k iterations for a 200 MB download, causing apparent hangs when callers materialise the full stream with b''.join(view.csv). Closes #1374 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
10703d8 to
695f784
Compare
Code ReviewSummary: Raises the Strengths
Issues / GapsInconsistent chunk size left in place elsewhere. The same hardcoded
Worth confirming whether No test asserts the new chunk size. The PR description's test plan checkbox is also still unchecked. Correctness
🤖 Generated with Claude Code |
|
@jacalata kind of funny how claude code keeps finding issues with a claude code aided PR :) Consider doing those consistency points if they make sense. |
…nd DownloadableMixin Views, CustomViews, Workbooks, Datasources, and Flows all had a hardcoded 1024-byte chunk size for streaming downloads, which caused effective hangs on large (200+ MB) view exports (#1374). - DownloadableMixin._download_content: use config.CHUNK_SIZE_MB * BYTES_PER_MB instead of hardcoded 1024. Benefits workbooks, datasources, flows. - DownloadableMixin._stream_content: new sibling method for endpoints that need Iterator[bytes] rather than file-writing. Same chunk-size config. - Views and CustomViews: migrate to _stream_content, removing their duplicate streaming loops. Callers that materialize CSV/Excel via b"".join(view.csv) or iterate lazily continue to work exactly the same way; only the internal chunk size changes.
|
Yea, clearly I need to run a more adversarial set of claudes! Turned out the consistency points were important: since I opened this PR, #1803 landed on development and extracted the streaming-to-file logic used by workbooks/datasources/flows into Expanded the PR scope:
Callers using |
There was a problem hiding this comment.
Pull request overview
This PR refactors how binary and export content is streamed from the server by routing view/custom-view CSV/Excel downloads through a shared DownloadableMixin._stream_content helper and by changing the iter_content() chunk size used during streaming.
Changes:
- Introduces
DownloadableMixin._stream_content()and updates Views/CustomViews CSV/Excel fetchers to use it. - Updates
DownloadableMixin._download_content()to stream using a configurable chunk size instead of the previous 1 KB chunks. - Adds regression tests asserting that streaming/download paths call
requests.Response.iter_content()with the configured chunk size.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/test_workbook.py | Adds a regression test ensuring workbook downloads stream using the configured chunk size. |
| test/test_view.py | Adds a regression test ensuring view CSV streaming uses the configured chunk size. |
| tableauserverclient/server/endpoint/views_endpoint.py | Mixes in DownloadableMixin and routes CSV/Excel view streaming through _stream_content(). |
| tableauserverclient/server/endpoint/endpoint.py | Adds _stream_content() and changes streaming chunk size logic in the shared download/stream helpers. |
| tableauserverclient/server/endpoint/custom_views_endpoint.py | Mixes in DownloadableMixin and routes custom-view CSV streaming through _stream_content(). |
Suppressed comments (1)
tableauserverclient/server/endpoint/endpoint.py:405
- _stream_content also uses config.CHUNK_SIZE_MB * BYTES_PER_MB as its iter_content() chunk size, which is currently tuned/documented for upload chunking (TSC_CHUNK_SIZE_MB) and defaults to 50 MB. For view/custom-view CSV/Excel streaming this is likely too large for responsive iteration; consider using a smaller, download-specific chunk size (e.g., 64 KiB) and keeping upload chunk sizing separate.
write it to a file. Complements _download_content, which writes to disk.
"""
chunk_size = config.CHUNK_SIZE_MB * BYTES_PER_MB
with closing(
self.get_request(url, request_object=request_object, parameters={"stream": True}) # type: ignore[attr-defined]
) as server_response:
yield from server_response.iter_content(chunk_size)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m = Message() | ||
| m["Content-Disposition"] = server_response.headers["Content-Disposition"] | ||
| filename = m.get_filename(failobj="") | ||
| chunk_size = config.CHUNK_SIZE_MB * BYTES_PER_MB | ||
| if isinstance(filepath, _io_types_w): |
There was a problem hiding this comment.
Correct that the code uses config.CHUNK_SIZE_MB * BYTES_PER_MB (default 50 MB), not the 65,536 bytes the PR body still claims. Sequence of events: the PR started as a bump from 1024 to 65,536 in the two view helpers, but grew into unifying chunk sizing across all download paths via DownloadableMixin._stream_content + _download_content using the same config.CHUNK_SIZE_MB knob. The PR body is out of date; updating it now.
Two things worth separating:
-
Consistency: this PR makes view/custom-view streaming use the same chunk size as workbook/datasource/flow downloads (pre-existing use of
config.CHUNK_SIZE_MBon line 378). That's intentional and correct — one knob for everything, tunable viaTSC_CHUNK_SIZE_MB. -
Whether 50 MB is the right default: legitimate concern for interactive streaming of small CSVs (
iter_content(50 MB)on a 500 KB response yields one chunk with the whole body — functionally fine but defeats lazy processing). This affects both the new_stream_contentand the pre-existing_download_content. Retuning the default is a separate change, worth doing but not scoped to this PR.
Summary
Unifies streaming chunk size across all download paths in
Endpoint. Previously:DownloadableMixin._download_content(workbook / datasource / flow) used a hard-coded 1024-byte chunk since the mixin was introduced.Views._get_view_csv/_get_view_excelandCustomViews._get_custom_view_csvalso used hard-coded 1024 bytes.TSC_CHUNK_SIZE_MBenv var /config.CHUNK_SIZE_MBknob existed for uploads (fileuploads_endpoint) but was never applied to downloads.This PR:
DownloadableMixin._stream_content(url, req_options) -> Iterator[bytes]for endpoints whose callers want to lazily iterate the response (Views, CustomViews)._download_contentand_stream_contentthroughconfig.CHUNK_SIZE_MB * BYTES_PER_MB(default 50 MB)._stream_content.Effect: the 1 KB default (which required ~200,000 generator iterations for a 200 MB CSV, causing apparent hangs when callers materialize the full stream via
b''.join(view.csv)) is replaced by the tunableTSC_CHUNK_SIZE_MBknob. The default of 50 MB is what upload paths already use; whether that default is right for interactive download streaming is a separate design question — see the Copilot-review thread and issue #1374 comment history.Closes #1374
Test plan
test_populate_csv,test_populate_excel,test_filter_excel,test_populate_csv_default_maxageall pass🤖 Generated with Claude Code