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.

Sign up for GitHub

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

Jump to bottom

Override neo4j user agent when driver is injected #243

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Override neo4j user agent when driver is injected #243

wants to merge 2 commits into from

Conversation

jonbesga
Copy link
Contributor

Description

Override neo4j user agent when driver is injected

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

  • 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

@jonbesga jonbesga requested a review from a team as a code owner January 13, 2025 23:23
@stellasia
Copy link
Contributor

LGTM. Just one point: I think we're doing things wrong here, by setting self.driver again, while it is already defined in the parent class (and only the parent class has this telemetry change, which makes sense). Do you think we can remove this line? (we have a similar pattern in the other external retrievers)

@jonbesga
Copy link
Contributor Author

LGTM. Just one point: I think we're doing things wrong here, by setting self.driver again, while it is already defined in the parent class (and only the parent class has this telemetry change, which makes sense). Do you think we can remove this line? (we have a similar pattern in the other external retrievers)

I think is fine because I'm modifying the driver object that is being passed around. If you check the id of the driver object that gets passed to the parent class and the validated_data.driver_model.driver that gets reassigned. They are the same.

oskarhane
oskarhane reviewed Jan 14, 2025
View reviewed changes
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Great!

Is there a way to verify that this work, in a test or so?

examples/customize/build_graph/components/writers/custom_writer.py
@@ -6,11 +6,12 @@
from neo4j_graphrag.experimental.components.kg_writer import KGWriter, KGWriterModel
from neo4j_graphrag.experimental.components.types import LexicalGraphConfig, Neo4jGraph
from pydantic import validate_call
from neo4j_graphrag.utils import telemetry
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this should be classified as telemetry, so this name is a bit misleading.
I'd say it's more of a driver configuration change. So maybe name the module driver_config or similar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stellasia stellasia stellasia left review comments

@oskarhane oskarhane oskarhane left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonbesga @stellasia @oskarhane