Skip to content

Conversation

@galshubeli
Copy link

@galshubeli galshubeli commented Mar 23, 2025

  • Description: This PR fixes conditions where FalkorDB queries contain FalkorDB node objects and converts them into a dictionary structure.
  • Issue: When the retrieval contains FalkorDB objects, the Q&A step cannot handle this context as natural language.
  • Dependencies: falkordb-py

If no one reviews your PR within a few days, please @-mention one of baskaryan, eyurtsev, ccurme, vbarda, hwchase17.

@vercel
Copy link

vercel bot commented Mar 23, 2025

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Mar 23, 2025 1:40pm

@dosubot dosubot bot added size:XS labels Mar 23, 2025
@galshubeli galshubeli changed the title Fix retrieval node objects community: Fix retrieval node objects Mar 23, 2025
Copy link
Collaborator

@ccurme ccurme left a comment

Choose a reason for hiding this comment

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

Can you add a test that demonstrates what is being fixed? I assume the string representation for Node is nonsensical? This is a change in the interface so we need to demonstrate that this is a bug fix (i.e., the existing behavior is not useful).

@ccurme ccurme added the needs test PR needs to be updated with tests label Mar 23, 2025
@galshubeli galshubeli closed this Mar 24, 2025
@galshubeli galshubeli reopened this Mar 24, 2025
@galshubeli
Copy link
Author

Can you add a test that demonstrates what is being fixed? I assume the string representation for Node is nonsensical? This is a change in the interface so we need to demonstrate that this is a bug fix (i.e., the existing behavior is not useful).

You're right. This is a bug fix for the string representation of nodes. In other graph providers, it is implemented in the graph file (see the image below). I believe the correct place for this is when the user needs the query for context. Do you agree?

Could you explain how to add a test for this?
image

@ccurme ccurme self-assigned this Mar 26, 2025
@galshubeli galshubeli requested a review from ccurme March 31, 2025 13:39
@ccurme
Copy link
Collaborator

ccurme commented Mar 31, 2025

@galshubeli For testing, we'd ideally add a unit test into a (new) file here. It should test what context is being supplied to a (fake) chat model. It looks like we return the context in intermediate steps, so you should be able to access that in the test.

The test should fail for the implementation that's currently on master, and demonstrate the impact of the change in this PR.

Copy link
Collaborator

@ccurme ccurme left a comment

Choose a reason for hiding this comment

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

@galshubeli we've now moved langchain-community into a standalone repo. Would you mind opening your PR there, along with a unit test?

https://github.com/langchain-ai/langchain-community

@ccurme ccurme closed this Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs test PR needs to be updated with tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants