Skip to content
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

Feature/add kg agent search to pipeline rebased 2 #483

Merged
merged 15 commits into from
Jun 19, 2024

Conversation

emrgnt-cmplxty
Copy link
Contributor

@emrgnt-cmplxty emrgnt-cmplxty commented Jun 18, 2024

🚀 This description was created by Ellipsis for commit 100f566

Summary:

Introduced KGAgentSearchPipe, updated r2r/main/r2r_client.py for streaming, and integrated the new pipe across various components, ensuring backward compatibility while updating specific files and tests.

Key points:

  • Introduced KGAgentSearchPipe to replace KGAgentPipe.
  • Updated r2r/main/r2r_client.py for synchronous and asynchronous streaming using R2RRAGRequest.
  • Integrated the new pipe across documentation, core abstractions, pipelines, providers, and tests.
  • Added encode_data and decode_data methods in Document class.
  • Updated file and document ingestion methods in r2r/main/r2r_app.py.
  • Included new request models like R2RIngestFilesRequest, R2RUpdateFilesRequest.
  • Ensured backward compatibility with existing functionality.
  • Updated tests in tests/test_end_to_end.py and tests/test_server_client.py to reflect changes.

Generated with ❤️ by ellipsis.dev

Copy link

vercel bot commented Jun 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
r2r-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 19, 2024 5:02pm

@emrgnt-cmplxty emrgnt-cmplxty marked this pull request as ready for review June 18, 2024 15:54
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 62212ba in 2 minutes and 57 seconds

More details
  • Looked at 3053 lines of code in 52 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_R0jUolKqI96dPqfZ


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.

)

extraction = result.choices[0].message.content
query = extraction.split("```cypher")[1].split("```")[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure to strip any leading or trailing whitespace from the extracted query to avoid potential issues during its execution.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Incremental review on 7449d1c in 1 minute and 43 seconds

More details
  • Looked at 106 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. r2r/main/r2r_client.py:149
  • Draft comment:
    The PR description mentions the introduction of KGAgentSearchPipe to replace KGAgentPipe, but the changes in this PR do not reflect this. Please ensure that the PR description accurately reflects the changes made or update the PR to include the missing changes.
  • Reason this comment was not posted:
    Confidence of 0% on close inspection, compared to threshold of 50%.

Workflow ID: wflow_veoufgAzhSG1HOd9


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 89bc9f4 in 2 minutes and 41 seconds

More details
  • Looked at 906 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_UW7uXaCoSOCs2vLc


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.

data=serialized_data,
headers={"Content-Type": "application/json"},
)
request = R2RUpdateDocumentsRequest(documents=documents, versions=None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion
request = R2RUpdateDocumentsRequest(documents=documents, versions=versions)

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 40744cb in 2 minutes and 45 seconds

More details
  • Looked at 1112 lines of code in 20 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_BOzmC1WOfxdW2CVl


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.

metadatas: Optional[list[dict]] = None


class R2RIngestFilesRequest(BaseModel):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The R2RIngestFilesRequest class has inconsistent types for document_ids and user_ids. They are defined as uuid.UUID in the class but are used as strings in various parts of the application. This can lead to type errors or unexpected behavior. Consider aligning the types across all usages.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Incremental review on 46e02f2 in 1 minute and 46 seconds

More details
  • Looked at 573 lines of code in 10 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. r2r/main/r2r_client.py:160
  • Draft comment:
    The parameter name streaming should be updated to stream to maintain consistency with the rest of the codebase and the PR description.
        stream: bool = False,
  • Reason this comment was not posted:
    Confidence of 0% on close inspection, compared to threshold of 50%.

Workflow ID: wflow_byMa9xGaVfAEikzI


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@emrgnt-cmplxty emrgnt-cmplxty merged commit 45b0036 into dev Jun 19, 2024
2 of 4 checks passed
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 100f566 in 2 minutes and 6 seconds

More details
  • Looked at 174 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_D4VdcUssPqfynpf6


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.


class R2RUpdateFilesRequest(BaseModel):
metadatas: Optional[list[dict]] = None
document_ids: Optional[uuid.UUID] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The document_ids field in R2RUpdateFilesRequest should likely be a list of UUIDs to support updating multiple files. Please adjust the type to Optional[list[uuid.UUID]] to align with typical use cases and existing handling logic in the application.

Suggested change
document_ids: Optional[uuid.UUID] = None
document_ids: Optional[list[uuid.UUID]] = None

@emrgnt-cmplxty emrgnt-cmplxty deleted the feature/add-kg-agent-search-to-pipeline-rebased-2 branch June 19, 2024 21:11
emrgnt-cmplxty added a commit that referenced this pull request Jun 21, 2024
* expand search pipeline

* checkin progress

* checkin progress

* grinding out

* make sure we can configure kg

* fix id error on server use

* checkin

* finish merge, add rag stream

* cleanup and pass tests

* up

* harmonize

* check in config refactor

* checkin work

* cleanup

* minor cleanups
emrgnt-cmplxty added a commit that referenced this pull request Jun 21, 2024
* quiet logger (#482)

* Add debug mode to Posthog (#480)

* Feature/add kg agent search to pipeline rebased 2 (#483)

* expand search pipeline

* checkin progress

* checkin progress

* grinding out

* make sure we can configure kg

* fix id error on server use

* checkin

* finish merge, add rag stream

* cleanup and pass tests

* up

* harmonize

* check in config refactor

* checkin work

* cleanup

* minor cleanups

* Add optional metadata to KG cookbook (#484)

* Add optional metadata to KG cookbook

* Align CB with script

* rm cruft and fix bug (#488)

* Feature/refactor app logic (#490)

* modularize app logic

* add management service

* add management service

* add management service

* Feature/move to singleton to prevent reload rebased (#491)

* modularize app logic

* add management service

* prevent r2r reload

* prevent r2r reload

* add back file (#492)

* add back file (#493)

* add back file

* fix bugs

* fix the routes (#496)

* rebase

* update docs

* cleanups

---------

Co-authored-by: Nolan Tremelling <34580718+NolanTrem@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant