Skip to content

Conversation

a-s-g93
Copy link
Contributor

@a-s-g93 a-s-g93 commented Jul 29, 2024

Description

Add an option to provide a custom prompt to the Text2Cypher Retriever. Doing so will ignore any provided graph schema or examples.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Documentation update
  • Project configuration change

Complexity

Low

Complexity:

How Has This Been Tested?

  • Unit tests
  • E2E tests
  • Manual tests

Checklist

The following requirements should have been met (depending on the changes in the branch):

no E2E tests needed, no examples written, no new files created

  • Documentation has been updated
  • Unit tests have been updated
  • E2E tests have been updated
  • Examples have been updated
  • New files have copyright header
  • CLA (https://neo4j.com/developer/cla/) has been signed
  • CHANGELOG.md updated if appropriate

@willtai
Copy link
Contributor

willtai commented Jul 30, 2024

Hey @a-s-g93, thanks for the contribution! To pass the CLA check you will need to sign our CLA, please follow the instructions here.

@a-s-g93
Copy link
Contributor Author

a-s-g93 commented Jul 30, 2024

Hey @a-s-g93, thanks for the contribution! To pass the CLA check you will need to sign our CLA, please follow the instructions here.

Hey @willtai I sent an email yesterday with all required info before submitting my PR. How long before CLA info is verified?

Copy link
Contributor

@willtai willtai left a comment

Choose a reason for hiding this comment

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

Nice work! I would also add another test that raises SearchValidationError when a wrong type is used for the custom prompt.

Something similar to this https://github.com/neo4j/neo4j-genai-python/blob/main/tests/unit/retrievers/test_text2cypher.py#L73

@a-s-g93
Copy link
Contributor Author

a-s-g93 commented Jul 31, 2024

Nice work! I would also add another test that raises SearchValidationError when a wrong type is used for the custom prompt.

Something similar to this https://github.com/neo4j/neo4j-genai-python/blob/main/tests/unit/retrievers/test_text2cypher.py#L73

Thanks for the feedback @willtai! Should I test the RetrieverInitializationError error instead since this is caught during initialization?

@alexthomas93
Copy link
Contributor

Nice work! I would also add another test that raises SearchValidationError when a wrong type is used for the custom prompt.
Something similar to this https://github.com/neo4j/neo4j-genai-python/blob/main/tests/unit/retrievers/test_text2cypher.py#L73

Thanks for the feedback @willtai! Should I test the RetrieverInitializationError error instead since this is caught during initialization?

Yes RetrieverInitializationError is the one to test for here @a-s-g93

@a-s-g93 a-s-g93 requested a review from willtai August 1, 2024 15:33
@willtai
Copy link
Contributor

willtai commented Aug 6, 2024

The PR E2E tests have been scaled down to avoid running out of disk space. Please rebase off the latest changes in main to apply these changes to your PR

@a-s-g93 a-s-g93 force-pushed the text2cypher-custom-prompt-option branch from 2d717a1 to 3773bc6 Compare August 6, 2024 13:08
@a-s-g93
Copy link
Contributor Author

a-s-g93 commented Aug 6, 2024

I can't pass the mypy pre-commit check due to the Unused "type: ignore" comment [unused-ignore] in the test_text2cypher.py file. These comments aren't mine. When I run the mypy check from command line I get a success message. I've tested this with Python 3.10, 3.11 and 3.12. I've tried reinstalling the pre-commit hook and reinstalling the project dependencies. Do you have a recommendation on how to address this?

Screenshot 2024-08-06 at 10 31 40 AM

@alexthomas93 alexthomas93 force-pushed the text2cypher-custom-prompt-option branch from 765dfaf to 3cd4644 Compare August 7, 2024 15:12
Copy link
Contributor

@alexthomas93 alexthomas93 left a comment

Choose a reason for hiding this comment

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

LGTM

@willtai willtai merged commit b4cde39 into neo4j:main Aug 7, 2024
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.

3 participants