Skip to content

Conversation

willtai
Copy link
Contributor

@willtai willtai commented Aug 8, 2024

Description

#89 A previous PR removed the query argument from GraphRAG's .search() which is a breaking change. This PR allows the argument query to still be used, but raises warnings for the user to start using query_text.

Type of Change

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

Complexity

Complexity: Low

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):

  • 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 willtai marked this pull request as ready for review August 8, 2024 10:41
@willtai willtai changed the title Make query backwards compatible Make GraphRAG search's query backwards compatible Aug 8, 2024
@willtai
Copy link
Contributor Author

willtai commented Aug 8, 2024

@CodiumAI-Agent /update_changelog

@CodiumAI-Agent
Copy link

Changelog updates: 🔄

2024-08-08

Changed

  • Made GraphRAG.search method backwards compatible with the query parameter, raising warnings to encourage using query_text instead.

to commit the new content to the CHANGELOG.md file, please type:
'/update_changelog --pr_update_changelog.push_changelog_changes=true'

@willtai willtai requested review from stellasia and a team August 8, 2024 10:55
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!

- Add optional custom_prompt arg to the Text2CypherRetriever class.


### Changed
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we add a "Deprecation notice" section to make it even more visible?

DeprecationWarning,
stacklevel=2,
)
elif isinstance(query, str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to test for the type of query here?


### Changed
- `GraphRAG.search` method first parameter has been renamed `query_text` (was `query`) for consistency with the retrievers interface.
- Made `GraphRAG.search` method backwards compatible with the query parameter, raising warnings to encourage using query_text instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need a line in changelog for that, maybe rephrasing the previous one would be nicer. Something like:

`query` parameter in GraphRAG.search` was deprecated and will be removed in the next major release. Use `query_text` instead. 

@willtai willtai merged commit 149d1e9 into neo4j:main Aug 8, 2024
a-s-g93 pushed a commit to a-s-g93/neo4j-genai-python-dev that referenced this pull request Sep 13, 2024
* Make query backwards compatible

* Update CHANGELOG
a-s-g93 pushed a commit to a-s-g93/neo4j-genai-python-dev that referenced this pull request Sep 13, 2024
* Make query backwards compatible

* Update CHANGELOG
@willtai willtai deleted the backward-compatibility-for-query-parameter branch October 1, 2024 11:02
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.

4 participants