Shreyas/KG Search Result model #937
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 6ce0d66 in 23 seconds
More details
- Looked at
139lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. py/sdk/models.py:194
- Draft comment:
Thedictmethod is redundant asBaseModelalready provides this functionality. Consider removing it. - Reason this comment was not posted:
Marked as duplicate.
2. py/core/base/abstractions/search.py:61
- Draft comment:
The attributesearch_resultinKGSearchResultis namedresultsinpy/sdk/models.py. Consider using consistent naming across the codebase to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is suggesting a change for consistency across files, which is outside the scope of this PR. The PR does not introduce or modify the 'results' attribute in 'py/sdk/models.py', so the comment is not directly relevant to the changes made in this PR. Additionally, the comment does not point out a definite issue within the current file, but rather a potential inconsistency across files.
I might be missing the context of how 'KGSearchResult' is used in conjunction with 'py/sdk/models.py'. However, the comment does not address a direct issue in the current file, which is the focus of the review.
Even if there is a naming inconsistency, it is not directly related to the changes made in this PR. The focus should be on the changes within the file being reviewed.
The comment should be removed as it addresses a cross-file naming inconsistency, which is outside the scope of this PR review.
3. py/sdk/models.py:193
- Draft comment:
The attributeresultsinKGSearchResultis namedsearch_resultinpy/core/base/abstractions/search.py. Consider using consistent naming across the codebase to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_FNeBggyVtf5MiWAl
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
py/core/base/abstractions/search.py
Outdated
| def __repr__(self) -> str: | ||
| return self.__str__() | ||
|
|
||
| def dict(self) -> dict: |
There was a problem hiding this comment.
The dict method is redundant as BaseModel already provides this functionality. Consider removing it.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 75eba79 in 31 seconds
More details
- Looked at
118lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_A0utwC8OhH6J7xeF
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| KGSearchResult = list[Tuple[str, list[Dict[str, Any]]]] | ||
| def __str__(self) -> str: | ||
| return f"LocalSearchResult(query={self.query}, search_result={self.search_result})" |
There was a problem hiding this comment.
The __str__ method is using search_result, which is not an attribute of KGLocalSearchResult. It should use entities, relationships, and communities instead.
shreyaspimpalgaonkar
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 40c227e in 13 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. py/core/configs/neo4j_kg.toml:30
- Draft comment:
Ensure that the removed parameters (e.g.,temperature,top_p,max_tokens_to_sample,stream,add_generation_kwargs) are not required elsewhere in the codebase, as their removal might lead to unexpected behavior. - Reason this comment was not posted:
Confidence changes required:50%
The removal of certain parameters in thekg.kg_extraction_configandkg.kg_search_configsections might lead to issues if these parameters are required elsewhere in the codebase.
Workflow ID: wflow_DcicZxSZCN7p4SiO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
emrgnt-cmplxty
added a commit
that referenced
this pull request
Aug 23, 2024
* Feature/merge graphrag group mgmt (#876) * add group ids to document abstraction, first steps * extend group permissions * up * add tests for new group features * up * fixup auth * onboard extensive regression tests * adding regression tests * finish tests * rm selenium * test observability * uncomment tests * checkin first set of group tests * modify search, passing vector tests * checkin work * full delete logic * update search to use new filters * check in * Clean up * Check in * add search * tests/test_end_to_end.py::test_ingest_txt_document passing * cleanup logging * make schemas explicit * move to run logger abstraction * cleanup some test workflows * revive tests * tweak to pass tests * tweak rrf * finish hybrid search cleanup * fixup on regr tests, regen payloads * refresh payloads * refactor api model * Feature/refactor api model (#868) * cleanup imports * flake and cleanup * coherent global import / export structure * add ingestion response models * add management response models * cleanups * checkin work on routes * remove request models * last fixes * merge * add user / group gating * working test groups * updating client --------- Co-authored-by: NolanTrem <34580718+NolanTrem@users.noreply.github.com> * Clean up API (#878) * Get running * fixes in sdk * Add in more fixes * Feature/merge dev owen changes (#880) * add group ids to document abstraction, first steps * extend group permissions * up * add tests for new group features * up * fixup auth * onboard extensive regression tests * adding regression tests * finish tests * rm selenium * test observability * uncomment tests * checkin first set of group tests * modify search, passing vector tests * checkin work * full delete logic * update search to use new filters * check in * Clean up * Check in * add search * tests/test_end_to_end.py::test_ingest_txt_document passing * cleanup logging * make schemas explicit * move to run logger abstraction * cleanup some test workflows * revive tests * tweak to pass tests * tweak rrf * finish hybrid search cleanup * fixup on regr tests, regen payloads * refresh payloads * refactor api model * Feature/refactor api model (#868) * cleanup imports * flake and cleanup * coherent global import / export structure * add ingestion response models * add management response models * cleanups * checkin work on routes * remove request models * last fixes * merge * add user / group gating * working test groups * updating client * rename service to restructure * add get documents for group endpoint * fix client bugs * return delete format * merge cleanups * merge * finalize --------- Co-authored-by: NolanTrem <34580718+NolanTrem@users.noreply.github.com> * Shreyas/graphrag test (#881) * add group ids to document abstraction, first steps * extend group permissions * up * add tests for new group features * up * fixup auth * onboard extensive regression tests * adding regression tests * finish tests * rm selenium * test observability * uncomment tests * checkin first set of group tests * modify search, passing vector tests * checkin work * full delete logic * update search to use new filters * check in * Clean up * Check in * add search * tests/test_end_to_end.py::test_ingest_txt_document passing * cleanup logging * make schemas explicit * move to run logger abstraction * cleanup some test workflows * revive tests * tweak to pass tests * tweak rrf * finish hybrid search cleanup * fixup on regr tests, regen payloads * refresh payloads * refactor api model * Feature/refactor api model (#868) * cleanup imports * flake and cleanup * coherent global import / export structure * add ingestion response models * add management response models * cleanups * checkin work on routes * remove request models * last fixes * merge * add user / group gating * sync * enrich * up * fix global search * rag * remove client.py * rm configs * rm configs --------- Co-authored-by: emrgnt-cmplxty <owen@algofi.org> Co-authored-by: NolanTrem <34580718+NolanTrem@users.noreply.github.com> Co-authored-by: emrgnt-cmplxty <68796651+emrgnt-cmplxty@users.noreply.github.com> * Feature/fix embedding pipe (#882) * up * fixup concurrency * fix ollama embeddings * fix batching with ollama * checkin all cleanups * rm kg cruft (#884) * rm kg cruft * tweaks * tweak 2 (#885) * Feature/fix retrieval endpoint cruft (#887) * tweak 2 * fix retrieval endpoint descriptions * Python SDK (#886) Clean up Python SDK and routes * Separate out SDK, add js and go sdk to monorepo (#888) * Add r2r-js sdk * Add go sdk * Pull out python sdk * remove venv * Update packages * Check in fixes * Remove alembic dependencies * Feature/merge w nolan (#894) * cleanup hybrid search * cleanups in * Fix structure * Make graspologic optional * fix rag stream (#895) * add py r2r (#896) * Clean up (#897) * fix agent (#898) * define `RAGAgentResponse` (#899) * Shreyas/unstructured (#900) * api + oss lib * rm pdb * rm poetry lock * update version * fixes * Feature/cleanup client obj logic (#901) * define `RAGAgentResponse` * cleanup client logic * Shreyas/tests (#889) * init * tests * rename service * api model * add * merge * rm restructure router * print descriptions * Refactor CLI (#903) * Rm files readded by git (#904) * Remove Execution Wrapper (#905) * Rm files readded by git * Fix merge botch * Feature/fix auth revive tests rebased (#906) * adding the client touch ups * fix auth, revive tests * add back tests * uncomment run auth workflow * decruft * refresh test kg * fixup toml (#908) * Feature/fix ingestion update (#909) * fixup toml * fix update * Fix CLI Tests (#912) Fix CLI tests * Shreyas/kg runtime cfg (#913) add kg runtime config * rename kgenrichmentresponse (#914) * Feature/add nltk hybrid expansion rebased (#917) * expand hybrid search with nltk * cleanups * cleanup hybrid search * format * add setup.py * update * add script (#918) * Fix bug in document chunks (#921) * Fix bug in update files (#923) * Shreyas/unstructured (#922) * fix dockerfiles * adding config * fix paths * mv unstructured dep to docker * clean * Update docker_utils.py * Update unstructured_parsing.py * Update r2r_chunking.py * Update app_entry.py * Feature/repair logging (#925) * fixing logs * fix * rm double logging (#929) * Configs (#926) * Fix config logic * Update config * Clean up cli entry point * Disable SSL when installing nltk wordnet (#930) * Fix analytics endpoint * Update OpenAI sdk calls (#933) * Feature/revive advanced rag (#932) * rm double logging * revive advanced rag examples * merge (#934) * sync model (#935) * Feature/remove version from ingestion end pt (#936) * sync model * remove ability to set version * tweak versions impl * fix version bug * Move docker (#938) * Move docker * remove from root * Clean up sdk/restructure.py * Fix js tests, completion scoring (#939) * Shreyas/unstructured docker image (#940) unstructured docker image * Update JS (#941) * Update models (#942) * Feature/complete group logic (#945) * fix group logic * up * Fix Dockerbuild, Symlink Readme (#944) * Add back tast prompt override and include title if availible * Fix docker, sym link readme * Fix compose file path * Shreyas/KG Search Result model (#937) * return type to kg_search_result * add model * local and global results * modify config * refresh should not be gated by auth (#946) * Linting sync (#947) * Remove email from refresh (#948) * Fix link to image * Feature/rm print cruft rebase (#953) * refresh should not be gated by auth * rm print cruft * black and sort * merge * rm * update api return type * Update Actions (#954) * Update Github Actions (#956) * Update Actions * Update actions * Shreyas/kgsearchresult model (#957) * return type to kg_search_result * add model * local and global results * modify config * add models * up * fix config path * fix models * Login and refresh token bug (#959) * Update Actions * Fix bug in login with refresh token * Point pytest to linux (#960) * collection docs (#955) * Feature/merge dev to main (#962) * merge dev and main * git rm * add back collection fix * fix docker builds (#963) * Running unstructured docker + code cleanups (#964) * Small bugfixes on prompts, return types (#965) * Fix failing CLI tests * NPM publish action * remove tarball * Feature/fix dev tests (#966) * update auth tests * fix tests * back and sort * decruft * revert back to gpt-4o --------- Co-authored-by: NolanTrem <34580718+NolanTrem@users.noreply.github.com> Co-authored-by: Shreyas Pimpalgaonkar <shreyas.gp.7@gmail.com>
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.
Summary:
Refactored
KGSearchResultinto a class-based model, introducedKGLocalSearchResultandKGGlobalSearchResult, updated search methods, and cleanedneo4j_kg.toml.Key points:
KGSearchResultto a class-based model inpy/core/base/abstractions/search.pyandpy/sdk/models.py.KGLocalSearchResultandKGGlobalSearchResultfor local and global search results.local_searchandglobal_searchmethods to returnKGLocalSearchResultandKGGlobalSearchResultinstances.KGSearchResultnow aggregateslocal_resultandglobal_result.__str__,__repr__, anddictmethods for better representation and conversion.py/core/configs/neo4j_kg.toml.Generated with ❤️ by ellipsis.dev