Skip to content
This repository was archived by the owner on May 5, 2025. It is now read-only.

Clean up Storage code#570

Merged
Swatinem merged 2 commits intomainfrom
swatinem/cleanup-storage
Mar 17, 2025
Merged

Clean up Storage code#570
Swatinem merged 2 commits intomainfrom
swatinem/cleanup-storage

Conversation

@Swatinem
Copy link
Copy Markdown
Contributor

@Swatinem Swatinem commented Mar 12, 2025

This removes the old storage abstractions including all their tests.

And also inlines the minio adapter read/write implementation back into the base class.

@Swatinem Swatinem requested review from a team and joseph-sentry March 12, 2025 12:22
@Swatinem Swatinem self-assigned this Mar 12, 2025
@ghost
Copy link
Copy Markdown

ghost commented Mar 12, 2025

⏳ Sentry is currently working on detecting potential issues in your PR ⏳

Results will appear when the job is complete.

@Swatinem Swatinem force-pushed the swatinem/hardcode-newwrite branch from 3cc67a3 to 4ecfeb3 Compare March 12, 2025 12:25
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 12, 2025

CodSpeed Performance Report

Merging #570 will not alter performance

Comparing swatinem/cleanup-storage (fb2c910) with main (e30c1c7)

Summary

✅ 9 untouched benchmarks

Base automatically changed from swatinem/hardcode-newwrite to main March 13, 2025 08:53
@Swatinem Swatinem force-pushed the swatinem/cleanup-storage branch from b3e864e to 2afa495 Compare March 13, 2025 09:34
@Swatinem Swatinem marked this pull request as ready for review March 13, 2025 09:34
This removes the old storage abstractions including all their tests.

And also inlines the minio adapter read/write implementation back into the base class.
@Swatinem Swatinem force-pushed the swatinem/cleanup-storage branch from 2afa495 to 9ea3fc9 Compare March 13, 2025 10:21
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 82.14286% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.94%. Comparing base (284b92c) to head (fb2c910).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
shared/storage/minio.py 82.14% 7 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #570      +/-   ##
==========================================
+ Coverage   88.62%   88.94%   +0.31%     
==========================================
  Files         462      457       -5     
  Lines       13183    12934     -249     
  Branches     1513     1484      -29     
==========================================
- Hits        11684    11504     -180     
+ Misses       1181     1123      -58     
+ Partials      318      307      -11     
Flag Coverage Δ
shared-docker-uploader 88.94% <82.14%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Contributor

@joseph-sentry joseph-sentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some comments about possible future improvements

Comment thread shared/storage/minio.py Outdated
Comment thread shared/storage/minio.py
from shared.storage.base import (
CHUNK_SIZE,
PART_SIZE,
BaseStorageService,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be good in a follow up to remove the BaseStorageService as well, it was only useful to define an interface when we had multiple implementations, now that we only have one, it's not as usefule, same with the PresignedURLService as well

Comment thread shared/storage/minio.py
@@ -204,16 +215,58 @@ def write_file(
is_compressed: bool = False,
compression_type: str | None = "zstd",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess this can be done in a follow-up but we should "deprecate" this argument, maybe using overloads and https://typing.python.org/en/latest/spec/directives.html#example

@seer-by-sentry
Copy link
Copy Markdown

⏳ Sentry is currently working on detecting potential issues in your PR ⏳

Results will appear when the job is complete.

@codecov-notifications
Copy link
Copy Markdown

codecov-notifications Bot commented Mar 14, 2025

Codecov Report

Attention: Patch coverage is 82.14286% with 10 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
shared/storage/minio.py 82.14% 7 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

@Swatinem
Copy link
Copy Markdown
Contributor Author

Thanks for the suggestions. I removed the instrumentation from this PR.

And I agree that merging / removing the abstract base class / inferface is a good followup that I do want to do, along with cleaning up the various usages and parameters. That would likely need a few passes, as it also means having to audit/update all usage sites.

@Swatinem Swatinem added this pull request to the merge queue Mar 17, 2025
Merged via the queue into main with commit d84177b Mar 17, 2025
@Swatinem Swatinem deleted the swatinem/cleanup-storage branch March 17, 2025 08:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants