example DO NOT MERGE#104
Closed
joshuarobinson wants to merge 1 commit into
Closed
Conversation
Example of how we might add a more flexibile mapping system for sourcing byte ranges from different places.
📝 WalkthroughWalkthroughThis PR introduces range-mapped logical files: new types (FileRangeMapping, InlineBytesMapping, RangeMapping, RangeMappingMetadataProvider) and a MAPPED resolved-path state, client read logic to assemble bytes from ordered mappings, a concrete InvertedFileMetadataProvider implementing reverse-chunked mappings, and corresponding public exports. ChangesRange mapping feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SingleClient
participant MetadataProvider
participant StorageProvider
Caller->>SingleClient: read(logical_path, byte_range)
SingleClient->>MetadataProvider: realpath(logical_path)
MetadataProvider-->>SingleClient: ResolvedPath(state=MAPPED)
SingleClient->>SingleClient: _read_mapped(logical_path, byte_range)
SingleClient->>MetadataProvider: real_mappings(logical_path, byte_range)
MetadataProvider-->>SingleClient: ordered RangeMapping list
loop for each mapping
alt FileRangeMapping
SingleClient->>StorageProvider: read physical byte range
StorageProvider-->>SingleClient: bytes
else InlineBytesMapping
SingleClient->>SingleClient: append inline bytes
end
end
SingleClient-->>Caller: assembled bytes
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
multi-storage-client/src/multistorageclient/client/single.py (2)
255-271: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftSequential per-segment storage reads in
_read_mapped.Each
FileRangeMappingtriggers its own blockingget_objectcall in a simpleforloop. For providers that decompose a logical file into many small segments (e.g. the accompanyingInvertedFileMetadataProviderwith 1 KB chunks), this turns into many sequential round-trips on the read path. Consider fetchingFileRangeMappingsegments concurrently (the file already usesThreadPoolExecutorelsewhere, e.g._register_written_files) while preserving output order for the finalb"".join.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@multi-storage-client/src/multistorageclient/client/single.py` around lines 255 - 271, The `_read_mapped` path is issuing blocking `get_object` calls one segment at a time for each `FileRangeMapping`, which makes many small mapped files read very slowly. Update `single.py`’s `_read_mapped` to fetch `FileRangeMapping` segments concurrently, using the same `ThreadPoolExecutor` pattern already used in `_register_written_files`, while keeping the final byte assembly in the original mapping order. Preserve `InlineBytesMapping` handling as-is and make sure `b"".join` still receives parts in sequence.
436-441: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReuse the first
realpath()result here.
RangeMappingMetadataProvider.realpath()is called twice for non-MAPPEDreads; cache the firstResolvedPathand reuse itsexists/physical_pathfields to avoid the extra metadata lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@multi-storage-client/src/multistorageclient/client/single.py` around lines 436 - 441, The read path resolution in single.py is calling RangeMappingMetadataProvider.realpath() more than once for the same non-MAPPED request; update the logic around _resolve_read_path and the existing resolved variable so the first ResolvedPath is reused, and use its exists/physical_path fields instead of triggering a second metadata lookup.multi-storage-client/src/multistorageclient/providers/inverted_metadata.py (1)
108-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
list_objectsis missing type annotations present on every other overridden method.
path,start_after,end_at, andattribute_filter_expressionhave no type hints here, unlikeget_object_metadata,glob,generate_physical_path, etc. in the same class. As per coding guidelines, "Python code must be enforced by ruff (formatting + linting) and pyright (type checking)" — untyped parameters are likely to be flagged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@multi-storage-client/src/multistorageclient/providers/inverted_metadata.py` around lines 108 - 109, The InvertedMetadataProvider.list_objects override is missing the same type annotations used by the other methods in this class, so add explicit types for path, start_after, end_at, and attribute_filter_expression while keeping the existing Iterator[ObjectMetadata] return type. Match the style of get_object_metadata, glob, and generate_physical_path so the method is fully typed and consistent with the class’s other overrides.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@multi-storage-client/src/multistorageclient/providers/inverted_metadata.py`:
- Around line 51-136: The public methods in InvertedMetadataProvider are missing
required docstrings. Add Sphinx-style docstrings to realpath and each overridden
MetadataProvider method in the “Unsupported MetadataProvider methods” section,
including list_objects, get_object_metadata, glob, generate_physical_path,
add_file, remove_file, commit_updates, is_writable, allow_overwrites, and
should_use_soft_delete, so their purpose and unsupported behavior are documented
consistently.
- Around line 93-100: The underflow chunk offset calculation in the inverted
metadata mapping is clamping in the wrong order, which can collapse a partial
read to offset 0. Update the logic in the chunk-mapping path that builds
FileRangeMapping entries so the base offset is clamped before adding clip_start,
or otherwise preserve clip_start for the smallest physical chunk. Use the
existing range construction around physical_offset, clip_start, and
Range(offset=..., size=...) to locate and fix the bug.
---
Nitpick comments:
In `@multi-storage-client/src/multistorageclient/client/single.py`:
- Around line 255-271: The `_read_mapped` path is issuing blocking `get_object`
calls one segment at a time for each `FileRangeMapping`, which makes many small
mapped files read very slowly. Update `single.py`’s `_read_mapped` to fetch
`FileRangeMapping` segments concurrently, using the same `ThreadPoolExecutor`
pattern already used in `_register_written_files`, while keeping the final byte
assembly in the original mapping order. Preserve `InlineBytesMapping` handling
as-is and make sure `b"".join` still receives parts in sequence.
- Around line 436-441: The read path resolution in single.py is calling
RangeMappingMetadataProvider.realpath() more than once for the same non-MAPPED
request; update the logic around _resolve_read_path and the existing resolved
variable so the first ResolvedPath is reused, and use its exists/physical_path
fields instead of triggering a second metadata lookup.
In `@multi-storage-client/src/multistorageclient/providers/inverted_metadata.py`:
- Around line 108-109: The InvertedMetadataProvider.list_objects override is
missing the same type annotations used by the other methods in this class, so
add explicit types for path, start_after, end_at, and
attribute_filter_expression while keeping the existing Iterator[ObjectMetadata]
return type. Match the style of get_object_metadata, glob, and
generate_physical_path so the method is fully typed and consistent with the
class’s other overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a5901396-8d33-41c0-86e2-d676262e8bd7
📒 Files selected for processing (5)
multi-storage-client/src/multistorageclient/__init__.pymulti-storage-client/src/multistorageclient/client/single.pymulti-storage-client/src/multistorageclient/providers/__init__.pymulti-storage-client/src/multistorageclient/providers/inverted_metadata.pymulti-storage-client/src/multistorageclient/types.py
Comment on lines
+51
to
+136
| def realpath(self, logical_path: str) -> ResolvedPath: | ||
| if logical_path in self._files: | ||
| physical_path, _ = self._files[logical_path] | ||
| return ResolvedPath(physical_path=physical_path, state=ResolvedPathState.MAPPED) | ||
| return ResolvedPath(physical_path=logical_path, state=ResolvedPathState.UNTRACKED) | ||
|
|
||
| def real_mappings(self, logical_path: str, byte_range: Optional[Range] = None) -> list[RangeMapping]: | ||
| """ | ||
| Return chunks of the physical file in reverse order, with every 5th chunk | ||
| replaced by inline zero bytes. | ||
|
|
||
| If *byte_range* is given, only return the segments that overlap the requested | ||
| logical range, clipped to its boundaries. | ||
| """ | ||
| physical_path, file_size = self._files[logical_path] | ||
| if file_size == 0: | ||
| return [] | ||
|
|
||
| num_chunks = math.ceil(file_size / CHUNK_SIZE) | ||
|
|
||
| # Logical byte range we need to cover. | ||
| logical_start = byte_range.offset if byte_range else 0 | ||
| logical_end = (byte_range.offset + byte_range.size) if byte_range else file_size | ||
|
|
||
| result: list[RangeMapping] = [] | ||
| logical_cursor = 0 | ||
|
|
||
| for i in range(num_chunks): | ||
| # Size of this logical chunk (last chunk may be smaller). | ||
| chunk_logical_size = min(CHUNK_SIZE, file_size - i * CHUNK_SIZE) | ||
| chunk_logical_end = logical_cursor + chunk_logical_size | ||
|
|
||
| # Skip chunks entirely outside the requested range. | ||
| if chunk_logical_end <= logical_start or logical_cursor >= logical_end: | ||
| logical_cursor = chunk_logical_end | ||
| continue | ||
|
|
||
| # Clip this chunk to the requested range. | ||
| clip_start = max(logical_cursor, logical_start) - logical_cursor | ||
| clip_end = min(chunk_logical_end, logical_end) - logical_cursor | ||
| clipped_size = clip_end - clip_start | ||
|
|
||
| if (i + 1) % 5 == 0: | ||
| result.append(InlineBytesMapping(data=b"\x00" * clipped_size)) | ||
| else: | ||
| # This chunk corresponds to the (i+1)-th block from the end of the file. | ||
| physical_offset = file_size - (i + 1) * CHUNK_SIZE + clip_start | ||
| # Clamp to 0 for the very last (smallest) chunk. | ||
| physical_offset = max(0, physical_offset) | ||
| result.append(FileRangeMapping(physical_path=physical_path, range=Range(offset=physical_offset, size=clipped_size))) | ||
|
|
||
| logical_cursor = chunk_logical_end | ||
|
|
||
| return result | ||
|
|
||
| # -- Unsupported MetadataProvider methods -- | ||
|
|
||
| def list_objects(self, path, start_after=None, end_at=None, include_directories=False, attribute_filter_expression=None, show_attributes=False) -> Iterator[ObjectMetadata]: | ||
| raise NotImplementedError | ||
|
|
||
| def get_object_metadata(self, path: str, include_pending: bool = False) -> ObjectMetadata: | ||
| raise NotImplementedError | ||
|
|
||
| def glob(self, pattern: str, attribute_filter_expression=None) -> list[str]: | ||
| raise NotImplementedError | ||
|
|
||
| def generate_physical_path(self, logical_path: str, for_overwrite: bool = False) -> ResolvedPath: | ||
| raise NotImplementedError | ||
|
|
||
| def add_file(self, path: str, metadata: ObjectMetadata) -> None: | ||
| raise NotImplementedError | ||
|
|
||
| def remove_file(self, path: str) -> None: | ||
| raise NotImplementedError | ||
|
|
||
| def commit_updates(self) -> None: | ||
| raise NotImplementedError | ||
|
|
||
| def is_writable(self) -> bool: | ||
| return False | ||
|
|
||
| def allow_overwrites(self) -> bool: | ||
| return False | ||
|
|
||
| def should_use_soft_delete(self) -> bool: | ||
| return False |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing docstrings on public methods.
realpath and every "Unsupported MetadataProvider methods" override (list_objects, get_object_metadata, glob, generate_physical_path, add_file, remove_file, commit_updates, is_writable, allow_overwrites, should_use_soft_delete) lack docstrings. As per coding guidelines, "All public Python functions must have docstrings (Sphinx autodoc extracts them)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@multi-storage-client/src/multistorageclient/providers/inverted_metadata.py`
around lines 51 - 136, The public methods in InvertedMetadataProvider are
missing required docstrings. Add Sphinx-style docstrings to realpath and each
overridden MetadataProvider method in the “Unsupported MetadataProvider methods”
section, including list_objects, get_object_metadata, glob,
generate_physical_path, add_file, remove_file, commit_updates, is_writable,
allow_overwrites, and should_use_soft_delete, so their purpose and unsupported
behavior are documented consistently.
Source: Coding guidelines
Comment on lines
+93
to
+100
| if (i + 1) % 5 == 0: | ||
| result.append(InlineBytesMapping(data=b"\x00" * clipped_size)) | ||
| else: | ||
| # This chunk corresponds to the (i+1)-th block from the end of the file. | ||
| physical_offset = file_size - (i + 1) * CHUNK_SIZE + clip_start | ||
| # Clamp to 0 for the very last (smallest) chunk. | ||
| physical_offset = max(0, physical_offset) | ||
| result.append(FileRangeMapping(physical_path=physical_path, range=Range(offset=physical_offset, size=clipped_size))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Clamp-order bug corrupts partial reads into the underflow chunk.
physical_offset = file_size - (i + 1) * CHUNK_SIZE + clip_start is computed and then clamped with max(0, physical_offset). For the physically-first (logically-last) undersized chunk, the base term is negative; if a byte_range starts partway into that chunk (clip_start > 0), adding clip_start before clamping causes the clamp to discard the offset entirely, collapsing the result to 0 instead of clip_start. This returns the wrong physical bytes for any range read that begins inside that chunk.
🐛 Proposed fix: clamp the base offset before adding `clip_start`
- physical_offset = file_size - (i + 1) * CHUNK_SIZE + clip_start
- # Clamp to 0 for the very last (smallest) chunk.
- physical_offset = max(0, physical_offset)
+ # Clamp the chunk's base offset to 0 for the very last (smallest) chunk,
+ # then add clip_start so partial-range reads land at the correct position.
+ physical_offset = max(0, file_size - (i + 1) * CHUNK_SIZE) + clip_startWould you like me to draft a regression test covering a byte_range that starts partway into the underflow chunk?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (i + 1) % 5 == 0: | |
| result.append(InlineBytesMapping(data=b"\x00" * clipped_size)) | |
| else: | |
| # This chunk corresponds to the (i+1)-th block from the end of the file. | |
| physical_offset = file_size - (i + 1) * CHUNK_SIZE + clip_start | |
| # Clamp to 0 for the very last (smallest) chunk. | |
| physical_offset = max(0, physical_offset) | |
| result.append(FileRangeMapping(physical_path=physical_path, range=Range(offset=physical_offset, size=clipped_size))) | |
| if (i + 1) % 5 == 0: | |
| result.append(InlineBytesMapping(data=b"\x00" * clipped_size)) | |
| else: | |
| # This chunk corresponds to the (i+1)-th block from the end of the file. | |
| # Clamp the chunk's base offset to 0 for the very last (smallest) chunk, | |
| # then add clip_start so partial-range reads land at the correct position. | |
| physical_offset = max(0, file_size - (i + 1) * CHUNK_SIZE) + clip_start | |
| result.append(FileRangeMapping(physical_path=physical_path, range=Range(offset=physical_offset, size=clipped_size))) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@multi-storage-client/src/multistorageclient/providers/inverted_metadata.py`
around lines 93 - 100, The underflow chunk offset calculation in the inverted
metadata mapping is clamping in the wrong order, which can collapse a partial
read to offset 0. Update the logic in the chunk-mapping path that builds
FileRangeMapping entries so the base offset is clamped before adding clip_start,
or otherwise preserve clip_start for the smallest physical chunk. Use the
existing range construction around physical_offset, clip_start, and
Range(offset=..., size=...) to locate and fix the bug.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Example of how we might add a more flexibile mapping system for sourcing byte ranges from different places.
Description
Change description.
{Relates to/Closes} {Task ID}.
Checklist
.release_notes/.unreleased.mdmulti-storage-client/pyproject.toml.release_notes/.unreleased.md.release_notes/{bumped package version}.mdfile.Summary by CodeRabbit
New Features
Bug Fixes