-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat: zstd-compress requests over 1MB #559
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Experiment ResultsExperiment 1: air-passengersDescription:
Results:
Plot:Experiment 2: air-passengersDescription:
Results:
Plot:Experiment 3: electricity-multiple-seriesDescription:
Results:
Plot:Experiment 4: electricity-multiple-seriesDescription:
Results:
Plot:Experiment 5: electricity-multiple-seriesDescription:
Results:
Plot: |
setup.py
Outdated
@@ -48,6 +48,7 @@ | |||
"tenacity", | |||
"tqdm", | |||
"utilsforecast>=0.2.8", | |||
"zstandard", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also set httpx[zstd]
to ensure correct version is installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd be if we were getting zstd-compressed responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it seems httpx automatically adds accept-encoding: 'zstd'
to the headers if zstandard is installed and modal automatically compresses the responses when it sees this, so it's indeed worth adding. Thanks!
@@ -42,8 +42,7 @@ | |||
python_requires=">=3.9", | |||
install_requires=[ | |||
"annotated-types", | |||
"coreforecast>=0.0.14", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added in a bad merge in #547
" httpx.Client = original_client\n", | ||
"\n", | ||
"# this produces a 1MB payload\n", | ||
"series = generate_series(250, n_static_features=2)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it only compresses above 1 mb? So a 1 mb payload shouldn't compress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, two insignificant comments:
- Test is with 1 MB but I thought the idea that it doesn't compress up to (and including) 1 MB?
- Out of curiosity why 1MB as threshold?
I meant over 1MB in the comment, it's just slightly above it. The threshold is because at 1MB it takes around 1ms to compress and it's most likely worth it due to the savings in bandwidth, since it always compresses to less than half the original size. |
Automatically compresses payloads over 1MB using zstandard. This should help reduce the time when sending large payloads over slow internet connections.