Add support for more input sources in DirectoryReader #283
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
…utils in DirectoryReader
|
This pull request has been linked to Shortcut Story #40938: Object API: Add support for importing from group of TileDB Files. |
|
This pull request has been linked to Shortcut Story #42043: Trigger Task Graph for Indexing (Ingestion). |
Shelnutt2
reviewed
Mar 15, 2024
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
Shelnutt2
reviewed
Mar 27, 2024
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
| # Stop if we have found max_count files | ||
| if max_files is not None and len(results) >= max_files: | ||
| return | ||
| obj_type = tiledb.object_type(uri) |
There was a problem hiding this comment.
This needs a scoped context. Likely taking a context as a function parameter would work to pass it as a scoped ctx.
There was a problem hiding this comment.
Removed all config and context passing from DirectoryReaders.
Readers shouldn't own or manage the TileDB configuration. Rather the config is passed through the ingestion or index functions. Some reason for this:
- Reader arguments are serialized and stored in group metadata. Serializing TileDB config would present a risk of exposing user secrets.
- Multiple users can use the same index with different configs (i.e. each user having their credentials for s3). The Reader and its arguments are the same for all users of an Index
How it works currently:
- Config is passed to
ObjectIndexmethods (__init__,create,update_index). - We perform all reader operations within
with tiledb.scope_ctx(ctx_or_config=config) - Readers should expect a
default_ctxto be set appropriately
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledb/vector_search/object_readers/directory_reader.py
Outdated
Show resolved
Hide resolved
|
Great work, thanks for the changes! I've left some minor comments, mostly around the context handling. |
jparismorgan
approved these changes
Mar 27, 2024
jparismorgan
approved these changes
Mar 27, 2024
NikolaosPapailiou
deleted the
npapa/sc-40938/group-and-directory-listing
branch
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.
This adds support for listing TileDB groups
It also uses the same listing argument structure as the utilities used in VCF ingestion
[sc-42043]