feat: improvement of Ray sink API #2237
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
Add LanceOperation, Add commit for Append Operation. Pass Fragment as JSON string between Rust/Java. --------- Co-authored-by: Lei Xu <lei@lancedb.com>
eddyxu
force-pushed
the
lei/scale_ray_sink
branch
from
9e98ecb
to
c9a4130
Compare
eddyxu
requested review from
westonpace,
changhiskhan,
wjones127 and
chebbyChefNEQ
wjones127
changed the title
wjones127
reviewed
Apr 22, 2024
Comment on lines
+78
to
+80
| max_bytes_per_file = ( | ||
| DEFAULT_MAX_BYTES_PER_FILE if max_bytes_per_file is None else max_bytes_per_file | ||
| ) |
There was a problem hiding this comment.
Why not just put DEFAULT_MAX_BYTES_PER_FILE in the signature as the default value?
There was a problem hiding this comment.
So using None here we can later detect whether users specify this value or not. During a benchmark, the default 90GB causes OOM (now we know it was a bug in arrow. This allows us to provide a better value later.
There was a problem hiding this comment.
I see. Seems like a good thing to write a TODO comment for.
python/python/lance/ray/sink.py
Outdated
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| uri: str, | ||
| *, | ||
| transform: Callable[[pa.Table], Union[pa.Table, Generator]] = lambda x: x, | ||
| transform: Callable[[pa.Table], Union[pa.Table, Generator]] = None, |
There was a problem hiding this comment.
If you're going to make the default None, then you have to change the type accordingly:
Suggested change
| transform: Callable[[pa.Table], Union[pa.Table, Generator]] = None, | |
| transform: Optional[Callable[[pa.Table], Union[pa.Table, Generator]]] = None, |
python/python/lance/ray/sink.py
Outdated
| max_rows_per_file : int, optional | ||
| The maximum number of rows per file. Default is 1024 * 1024. | ||
| max_bytes_per_file : int, optional | ||
| The maximum number of bytes per file. Default is None. |
There was a problem hiding this comment.
Default is 90GB, not None, right?
Suggested change
| The maximum number of bytes per file. Default is None. | |
| The maximum number of bytes per file. Default is 90GB. |
python/python/lance/ray/sink.py
Outdated
| max_rows_per_file: int, optional | ||
| The maximum number of rows per file. Default is 1024 * 1024. | ||
| max_bytes_per_file: int, optional | ||
| The maximum number of bytes per file. Default is None. |
There was a problem hiding this comment.
Same here
Suggested change
| The maximum number of bytes per file. Default is None. | |
| The maximum number of bytes per file. Default is 90GB. |
wjones127
approved these changes
Apr 23, 2024
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.
max_bytes_per_filevia Ray sinkray.data.Dataset.write_lance()interface.