-
Notifications
You must be signed in to change notification settings - Fork 372
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
simplifying config #1941
Merged
Merged
simplifying config #1941
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
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.
👍 Looks good to me! Reviewed everything up to aa3bb61 in 2 minutes and 36 seconds
More details
- Looked at
6930
lines of code in105
files - Skipped
1
files when reviewing. - Skipped posting
14
drafted comments based on config settings.
1. py/core/providers/orchestration/simple.py:15
- Draft comment:
Several methods (get_worker, step, workflow, failure, start_worker) are left unimplemented (using 'pass'). Ensure these are implemented or clearly documented as stubs. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. py/migrations/versions/8077140e1e99_v3_api_database_revision.py:16
- Draft comment:
Migration script assumes table column renames and drops; verify that the schema matches production. Consider adding more comments if special handling is needed. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. py/sdk/base/base_client.py:24
- Draft comment:
The _get_auth_header() method raises an exception if both access token and API key are set. Consider logging a detailed error message for easier debugging in production. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. py/shared/utils/base_utils.py:112
- Draft comment:
There is a print(e) in the _handle_response() method. In production logging, use logger.error instead of print to avoid unwanted STDOUT. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. py/sdk/sync_methods/documents.py:108
- Draft comment:
Consider handling file open errors (e.g., file not found) in the create() method to ensure resource cleanup is robust. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. py/sdk/sync_methods/retrieval.py:71
- Draft comment:
In the completion() method, the FIXME comment indicates missing return type. It would be beneficial to specify and document the expected type. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
7. py/tests/integration/test_retrieval.py:90
- Draft comment:
Consider using async consumption for streaming tests instead of a synchronous consume_stream() helper to properly test async generators. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. py/tests/unit/test_config.py:209
- Draft comment:
In test_config_required_keys, consider handling cases when sections are not dicts more robustly instead of using hasattr(). - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
9. py/shared/abstractions/llm.py:104
- Draft comment:
In the GenerationConfig init, consider checking for a provided model argument more explicitly to avoid unexpected behavior. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
10. py/core/providers/database/prompts/graph_extraction.yaml:11
- Draft comment:
Typo: "entit" should be "entity" for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. py/migrations/versions/8077140e1e99_v3_api_database_revision.py:40
- Draft comment:
Migration check: Ensure that checking for the presence of column 'id' in the collections table is sufficient to decide that migration is not needed. Consider edge cases with custom table structures. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. py/sdk/sync_methods/documents.py:104
- Draft comment:
File handling: Consider using a 'with' statement to handle the temporary file to ensure it's closed properly even on exceptions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. tests/integration/test_indices.py:26
- Draft comment:
Test coverage: Several tests (e.g., create, delete index) are commented out. Re-enable these tests once stable to increase coverage. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. tests/integration/test_retrieval.py:90
- Draft comment:
Streaming tests: Instead of using a synchronous consumption loop, consider using asyncio for streaming tests to better simulate async behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_umPj7VxWCfMk3mY7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
Important
This pull request simplifies configuration management, enhances SDK functionalities, and updates core logic for graph extraction in the R2R codebase.
createSample
method inDocumentsSDK
to ingest sample documents.rawr
toreasoning_agent
inRetrievalSDK
.r2r.toml
andpyproject.toml
to reflect new LLM settings and defaults.graph_extraction_prompt
.KGExtractionStatus
withGraphExtractionStatus
and similar renames for consistency.GraphService
and related classes to use new graph extraction logic.This description was created by
for aa3bb61. It will automatically update as commits are pushed.