-
Notifications
You must be signed in to change notification settings - Fork 199
feat: support IndexRange and PartitionBlock seed selection strategy #8
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
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
b429464
add IndexRange and PartitionBlock
nabinchha 0d4b9a0
make check-all-fix
nabinchha 57d500b
add support for IndexRange and PartitionBlock
nabinchha 6bd7760
linting
nabinchha 12301b3
update tests + test e2e
nabinchha 03c048e
run ruff
nabinchha 6da2c3c
license check header
nabinchha 80fca51
partition_index -> index
nabinchha 4e3bd3e
update log message
nabinchha c32b278
Remove sub subquery alias notneeded
nabinchha 483363d
Optimize duckdb seed dataset select based on on limit and offset
nabinchha 9765643
Add docstring to seedconfig
nabinchha 98993de
add guide
johnnygreco b4be821
feat req updates
johnnygreco e9f97d6
git branch pattern update
johnnygreco 5765301
Update CONTRIBUTING.md
johnnygreco 2c38781
agent md blurb
johnnygreco f405dbd
pr feedback
johnnygreco 13f9527
some rewording
johnnygreco c84a70b
punctuation
johnnygreco 17657e6
missing quote
johnnygreco 6b14c31
Merge branch 'main' into nm/seed-config-partition-strategy
nabinchha File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import pytest | ||
|
|
||
| from data_designer.config.seed import IndexRange, PartitionBlock | ||
|
|
||
|
|
||
| def test_index_range_validation(): | ||
| with pytest.raises(ValueError, match="should be greater than or equal to 0"): | ||
| IndexRange(start=-1, end=10) | ||
|
|
||
| with pytest.raises(ValueError, match="should be greater than or equal to 0"): | ||
| IndexRange(start=0, end=-1) | ||
|
|
||
| with pytest.raises(ValueError, match="'start' index must be less than or equal to 'end' index"): | ||
| IndexRange(start=11, end=10) | ||
|
|
||
|
|
||
| def test_index_range_size(): | ||
| assert IndexRange(start=0, end=10).size == 11 | ||
| assert IndexRange(start=1, end=10).size == 10 | ||
| assert IndexRange(start=0, end=0).size == 1 | ||
|
|
||
|
|
||
| def test_partition_block_validation(): | ||
| with pytest.raises(ValueError, match="should be greater than or equal to 0"): | ||
| PartitionBlock(index=-1, num_partitions=10) | ||
|
|
||
| with pytest.raises(ValueError, match="should be greater than or equal to 1"): | ||
| PartitionBlock(index=0, num_partitions=0) | ||
|
|
||
| with pytest.raises(ValueError, match="'index' must be less than 'num_partitions'"): | ||
| PartitionBlock(index=10, num_partitions=10) | ||
|
|
||
|
|
||
| def test_partition_block_to_index_range(): | ||
| index_range = PartitionBlock(index=0, num_partitions=10).to_index_range(101) | ||
| assert index_range.start == 0 | ||
| assert index_range.end == 9 | ||
| assert index_range.size == 10 | ||
|
|
||
| index_range = PartitionBlock(index=1, num_partitions=10).to_index_range(105) | ||
| assert index_range.start == 10 | ||
| assert index_range.end == 19 | ||
| assert index_range.size == 10 | ||
|
|
||
| index_range = PartitionBlock(index=2, num_partitions=10).to_index_range(105) | ||
| assert index_range.start == 20 | ||
| assert index_range.end == 29 | ||
| assert index_range.size == 10 | ||
|
|
||
| index_range = PartitionBlock(index=9, num_partitions=10).to_index_range(105) | ||
| assert index_range.start == 90 | ||
| assert index_range.end == 104 | ||
| assert index_range.size == 15 |
Oops, something went wrong.
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.
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.
When the sampling strategy is "shuffle", does this shuffle the entire dataset before indexing, or does it shuffle the indexed array?
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 think it shuffles the indexed array because the where clause is most likely applied to filter first.
Uh oh!
There was an error while loading. Please reload this page.
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.
Is that what we want? Seems like "shuffle" should mean "shuffle the entire dataset" so that all records are equally likely to be present in a given indexed array. I'm not sure it actually matters too much, though, since all records are treated independently βΒ assuming the entire seed dataset is used.
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.
https://duckdb.org/docs/stable/sql/query_syntax/orderby
It's applied after the where clause filtering.
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.
It would be weird to shuffle first and then give exact indices ....
Uh oh!
There was an error while loading. Please reload this page.
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.
Let's think about the user's desired outcomes:
How this relates to
PartitionBlock:PartitionBlockaccording to the proportion of the size of the Block with respect to the full dataset. That should converge us to the same distribution of the original shuffle.I think that doing the proportional sampling per
PartitionBlockpost-partitioning would be desirable since it parallelizes easily vs needing to do an op on the full dataset.Uh oh!
There was an error while loading. Please reload this page.
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.
We resolve to indices automatically for the user, but it's in order based on how they define partitions.
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.
For posterity, we chatted on Slack about this. To solve for a weird distribution getting stuck in the the last partition ( the case of shuffle), we'd need to pre-shuffle the whole dataset. It is an expensive operation.
I ran a test to see how long it would take for duckdb to shuffle a really large dataset. As an example, we took the web step dataset that's about 164 GB.
TL;DR
duckdb took about a half hour to shuffle the entire dataset and write to 1000 partitions in a different destination:
srun Execution
Output
shuffle.py
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.
Ok I've optimized the select query to be based on limit and offset followed by order by. The previous implementation was still doing a full table scan. 483363d
Example run to pull a 1000 item slice from the 164GB partitioned dataset took <1s:
cc @eric-tramel @johnnygreco
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.