Repository navigation
Keep nested directories in merge_index shard paths - #23
Merged
Merged
Conversation
merge_index prefixed each shard basename with only the immediate parent directory of its partition index, so partitions nested more than one level under the merge root ended up with shard paths that do not exist, breaking downstream streaming with 404s (issue #959). Resolve each partition's directory relative to the merge root instead, falling back to the previous single-level behavior when a partition does not reside under the root. Add a spark-free nested-layout regression test and unit tests for the new path resolution helper.
Merged
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
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.
merge_indexdropped intermediate directories from shard paths when MDS partitions sat more than one level below the merge root:_merge_index_from_listrebuilt each merged basename asos.path.join(os.path.basename(p.parent), basename), which keeps exactly one directory level, so a partition atout/group1/subdir2/was listed assubdir2/shard.00000.mdsand the merged dataset could not find its shards. This imports mosaicml/streaming#983 by @discobot with the original commit kept so the author stays credited, and addresses upstream issue mosaicml/streaming#959._get_partition_dirname(index_file_url, out), which resolves each partition's directory relative to the merge root (local against the local root, remote against the remote root, same scheme and bucket only) and returnsNonewhen the partition is not underout._merge_index_from_list; partitions outsideoutfall back to the previous single-level behaviour, so existing one-level layouts are unchanged.test_get_partition_dirname(local, remote, tuple, root-level, cross-bucket and outside-root cases) andtest_merge_index_from_root_local_nested, which writes four partitions atout/group{1,2}/subdir{1,2}, merges, and checks the basenames and aStreamingDatasetread.Checked locally on Python 3.12:
SKIP=pyright pre-commit run --files streaming/base/util.py tests/test_util.pyis clean;pytest tests/test_util.py -k "merge_index or partition_dirname"gives 12 passed for the two new tests, and the nested test fails onmainwithexpected 4 shard files but got 2; the pre-existing Spark-backedtest_merge_index_from_list_localandtest_merge_index_from_root_localmatrices (26 cases) could not run here because the only JDK present (Homebrew OpenJDK 26) is rejected by PySpark 3.5, so those rely on CI.