Add use of zstd compression on compute services #336
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Update env files to include zstandard - Update set_task_result in compute api and client to handle base64 encoded data. Rather than JSON serialize the ProtocolDAGResult (PDR) and use this is a the intermediate format, instead: 1) create a keyed chain representation of the PDR 2) JSON serialize this representation 3) compress the utf-8 encoded bytes with zstandard 4) encode with base64 - Use the above base64 encoded data as the intermediate format and reverse the operations above to recover the PDR.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
==========================================
+ Coverage 80.35% 80.49% +0.14%
==========================================
Files 26 27 +1
Lines 3711 3743 +32
==========================================
+ Hits 2982 3013 +31
- Misses 729 730 +1 ☔ View full report in Codecov by Sentry. |
Use more bytes Move compression and decompression functions to new module Use latin-1 decoded bytes
If a decompression error is raised, assume that the original data was never compressed.
ianmkenney
force-pushed
the
feature/220-zstd-compression-compute-services
branch
from
4541b72 to
cce6e8d
Compare
Test getting extends ProtocolDAGResults as if they were stored through the old pdr.to_dict() -> json -> utf-8 encoded format. The new test can be removed in the next major release that drops the old format.
ianmkenney
force-pushed
the
feature/220-zstd-compression-compute-services
branch
from
cce6e8d to
b32f62d
Compare
To allow for better and clearer testing of result pushing and pulling, the act of executing a task and pushing its results were separated.
Code coverage was artificially low due to run test run order. A reset and reinitialization of the s3os_server shows the correct results.
It's more robust to paramterize the old tests to use the legacy kwarg for pushing results rather than writing a new test that covers less of the codebase.
ianmkenney
changed the title
dotsdl
approved these changes
Jan 24, 2025
There was a problem hiding this comment.
Looks fantastic @ianmkenney! Thanks for adding elegant compression-at-rest for ProtocolDAGResult objects!
|
@ianmkenney I made some changes as we discussed. If you agree with these changes and tests are passing, feel free to merge! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR closes #220. It modifies the behavior of set_task_results in the compute client and api to use compressed keyed chain representations of ProtocolDAGResults instead of simple JSON serialization as the intermediate format.