feat: document search sources string resolver #264
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
|
Code Coverage SummaryDiff against mainResults for commit: bf56aea Minimum allowed coverage is ♻️ This comment has been updated with latest results |
micpst
changed the title
- Basic file URI ingestion - Wildcard pattern matching
maciejklimek
force-pushed
the
feat/221-document-search-sources-string-resolver
branch
from
6be3c3f to
1b7bd93
Compare
maciejklimek
force-pushed
the
feat/221-document-search-sources-string-resolver
branch
from
d3b1e38 to
4505521
Compare
|
Trivy scanning results. .venv/lib/python3.10/site-packages/PyJWT-2.9.0.dist-info/METADATA (secrets)Total: 1 (MEDIUM: 1, HIGH: 0, CRITICAL: 0) MEDIUM: JWT (jwt-token) .venv/lib/python3.10/site-packages/litellm/llms/huggingface/huggingface_llms_metadata/hf_text_generation_models.txt (secrets)Total: 1 (MEDIUM: 0, HIGH: 0, CRITICAL: 1) CRITICAL: HuggingFace (hugging-face-access-token) .venv/lib/python3.10/site-packages/litellm/proxy/_types.py (secrets)Total: 1 (MEDIUM: 1, HIGH: 0, CRITICAL: 0) MEDIUM: Slack (slack-web-hook) |
ludwiktrammer
requested changes
Jan 2, 2025
Comment on lines
+152
to
+158
| documents: Either: | ||
| - A URI string (e.g., "gcs://bucket/*") to specify source location(s), or | ||
| - A sequence of documents or metadata of the documents to ingest | ||
| URI format depends on the source type, for example: | ||
| - "file:///path/to/files/*.txt" | ||
| - "gcs://bucket/folder/*" | ||
| - "huggingface://dataset/split/row" |
There was a problem hiding this comment.
It was very hard for me to parse and understand, since it looks like a one long list and the examples are not next to what they are examples for.
Maybe something liek this:
Suggested change
| documents: Either: | |
| - A URI string (e.g., "gcs://bucket/*") to specify source location(s), or | |
| - A sequence of documents or metadata of the documents to ingest | |
| URI format depends on the source type, for example: | |
| - "file:///path/to/files/*.txt" | |
| - "gcs://bucket/folder/*" | |
| - "huggingface://dataset/split/row" | |
| documents: Either: | |
| - A sequence of `Document`, `DocumentMetadata`, or `Source` objects | |
| - A source-specific URI string (e.g., "gcs://bucket/*") to specify source location(s), for example: | |
| - "file:///path/to/files/*.txt" | |
| - "gcs://bucket/folder/*" | |
| - "huggingface://dataset/split/row" |
| document_processor: The document processor to use. If not provided, the document processor will be | ||
| determined based on the document metadata. | ||
| """ | ||
| if isinstance(documents, str): | ||
| from ragbits.document_search.documents.source_resolver import SourceResolver |
There was a problem hiding this comment.
Why the conditional import? I think in a long run having the import run once during the start of the process will be more efficient, and will help avoid hard to debug errors.
|
|
||
| sources = await SourceResolver.resolve(documents) | ||
| else: | ||
| sources = documents # type: ignore[assignment] |
There was a problem hiding this comment.
To avoid having to disable type-checking, just add the typing (Sequence[DocumentMeta | Document | Source]) to the first assignment of sources
| super().__init_subclass__(**kwargs) | ||
| Source._registry[cls.class_identifier()] = cls | ||
| if cls.protocol is not None: | ||
| from ragbits.document_search.documents.source_resolver import SourceResolver |
There was a problem hiding this comment.
Again - we never have done this (conditional imports inside methods based on whether a particular feature is enabled) before in ragbits - switching to similar convention would require a discussion. I think there are clear cons.
Comment on lines
+239
to
+241
| if not path_obj.is_absolute(): | ||
| # For relative paths, use current directory as base | ||
| path_obj = Path.cwd() / path_obj |
There was a problem hiding this comment.
I'm pretty sure this is is the default behavior of the Path class, so I believe this conditional is not necessary
| path_obj = Path.cwd() / path_obj | ||
|
|
||
| if "*" in str(path_obj): | ||
| # If path contains wildcards, use its parent as base |
There was a problem hiding this comment.
This seems confusing to me, could you explain? I don't get how this code can lead to support for cases explained in the method's docstring (e.g. simple "*.py" or patterns with "?").
To be honest, in my mind the whole method could just be this one line:
return [cls(path=f) for f in Path().glob(pattern)]
| document_search = DocumentSearch.from_config(CONFIG) | ||
|
|
||
| # Test ingesting from URI with wildcard | ||
| await document_search.ingest(f"file://{temp_dir}/test*.txt") |
There was a problem hiding this comment.
It would be good to test other supported syntax options, for example file://{temp_dir}/test?.txt, file://*.txt, file://**/test?.txt
|
This PR should probably also include some documentation |
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.
Solves #221