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

perf: New neo4j csv publisher to improve performance using batched params #1957

Merged
merged 10 commits into from
Aug 16, 2022
Merged

perf: New neo4j csv publisher to improve performance using batched params #1957

merged 10 commits into from
Aug 16, 2022

Conversation

kristenarmes
Copy link
Contributor

@kristenarmes kristenarmes commented Aug 11, 2022 •
edited
Loading

Summary of Changes

  • Use unwind clause in the cypher queries when merging nodes and relations to be able to batch params and improve performance
  • Updated to use managed transactions in line with Neo4j's recommended best practices
  • Added PUBLISH_REVERSE_RELATIONSHIPS config
    • True (default): keep the existing behavior of publishing two way relations between nodes
    • False: only publish a one way relationship
  • Added PRESERVE_ADHOC_UI_DATA config
    • True (default): stops the publisher from updating a node or relationship created via the UI, e.g. a description or owner added manually by an Amundsen user. Such nodes/relationships will not have a published_tag property that is set by databuilder.
    • False: nothing changes

Tests

Basic unit tests for the publisher

Documentation

N/A

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does

@kristenarmes
New publisher using managed transactions, unwind statements, configur…
98fbe60 
…able two or one way relations

Signed-off-by: Kristen Armes <karmes@lyft.com>
@kristenarmes
Add logic for preserving adhoc ui data, and move write transactions l…
1bf9938 
…ogic to reusable function

Signed-off-by: Kristen Armes <karmes@lyft.com>
@boring-cyborg boring-cyborg bot added the area:databuilder From databuilder folder label Aug 11, 2022
@kristenarmes
Fixing flake8 error (not sure how it got through, not from my change)
02051bf 
Signed-off-by: Kristen Armes <karmes@lyft.com>
@kristenarmes
Pulling in a few of the latest changes from the original publisher an…
92d04f8 
…d lint

Signed-off-by: Kristen Armes <karmes@lyft.com>
@kristenarmes
Fix tests
aed878c 
Signed-off-by: Kristen Armes <karmes@lyft.com>
@kristenarmes kristenarmes marked this pull request as ready for review August 12, 2022 00:37
@kristenarmes kristenarmes requested a review from a team as a code owner August 12, 2022 00:37
dkunitsk
dkunitsk reviewed Aug 12, 2022
View reviewed changes
databuilder/databuilder/publisher/neo4j_csv_unwind_publisher.py Outdated

# CSV HEADER
# A header with this suffix will be pass to Neo4j statement without quote
UNQUOTED_SUFFIX = ':UNQUOTED'
Copy link

Choose a reason for hiding this comment

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

this i think does nothing and is no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into this, it is used to specify fields where the values are not strings, so like "order_pos:UNQUOTED" would be for numbers 1, 2, etc

Copy link

Choose a reason for hiding this comment

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

I can't find a reference for this, could you share. I recall looking this up a year+ ago finding it hasn't been doing anything and wasn't really a thing since maybe the earliest versions of neo4j. I tried just now again, and didn't find anything concrete.

Copy link

Choose a reason for hiding this comment

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

Nevermind I found it. Clearly I was dumb back then.

Copy link

Choose a reason for hiding this comment

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

Okay I just looked more. IIUC: in the publisher, all that's happening is this suffix is being removed so it doesn't end up ingested. At some point, it was doing something, but not anymore (please correct me if I'm wrong). Instead of continuing to support this, instead can we simply remove this behavior from the loader and then, likewise, from all publishers? I did a full code search and I'm quite sure this is safe! Maybe double check though.

dkunitsk
dkunitsk reviewed Aug 12, 2022
View reviewed changes
databuilder/databuilder/publisher/neo4j_csv_unwind_publisher.py Outdated
def get_scope(self) -> str:
return 'publisher.neo4j'

def _create_indices(self, node_file: str) -> None:
Copy link

Choose a reason for hiding this comment

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

idea: allow for list of pre and post-publish actions. and set this as a default pre-publish action

@ozandogrultan
Copy link
Contributor

What is the minimum Neo4j version required to be able to use this new publisher? I would suggest to document it if there is such requirement

@kristenarmes
Copy link
Contributor Author

kristenarmes commented Aug 12, 2022 •
edited
Loading

Hey @ozandogrultan, the Neo4j driver was upgraded in databuilder v7.0.0 (#1938) so it should work for anyone past that version. But I can make a note of it in the description too 👍

@kristenarmes
Addressing PR feedback and change index creation to use a managed tra…
2635bdc 
…nsaction

Signed-off-by: Kristen Armes <karmes@lyft.com>
@kristenarmes
Refactor props body function and separate out constants
d74943e 
Signed-off-by: Kristen Armes <karmes@lyft.com>
@kristenarmes
Addressing PR feedback plus refactoring
7ea393b 
Signed-off-by: Kristen Armes <karmes@lyft.com>
dkunitsk
dkunitsk reviewed Aug 15, 2022
View reviewed changes
Copy link

@dkunitsk dkunitsk left a comment

Choose a reason for hiding this comment

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

amazing. Minor comments raised. Very nearly done.

databuilder/databuilder/publisher/neo4j_csv_unwind_publisher.py Outdated Show resolved Hide resolved
databuilder/databuilder/publisher/neo4j_csv_unwind_publisher.py Outdated Show resolved Hide resolved
databuilder/databuilder/publisher/neo4j_csv_unwind_publisher.py Outdated Show resolved Hide resolved
databuilder/databuilder/publisher/neo4j_csv_unwind_publisher.py Outdated Show resolved Hide resolved
databuilder/databuilder/publisher/neo4j_csv_unwind_publisher.py Outdated Show resolved Hide resolved
databuilder/databuilder/publisher/publisher_config_constants.py Show resolved Hide resolved
@kristenarmes
Addressing PR feedback, more refactoring
8875336 
Signed-off-by: Kristen Armes <karmes@lyft.com>
dkunitsk
dkunitsk approved these changes Aug 16, 2022
View reviewed changes
Copy link

@dkunitsk dkunitsk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

😭 🥲 😭

databuilder/databuilder/utils/publisher_utils.py
tx.run(stmt, parameters=params)


def get_props_body_keys(record_keys: list,
Copy link

Choose a reason for hiding this comment

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

so good

databuilder/databuilder/publisher/neo4j_csv_unwind_publisher.py Outdated Show resolved Hide resolved
@kristenarmes
Minor fixes and bump version
ce017d1 
Signed-off-by: Kristen Armes <karmes@lyft.com>
@kristenarmes kristenarmes merged commit 09f2d92 into amundsen-io:main Aug 16, 2022
@kristenarmes kristenarmes deleted the new-neo4j-publisher branch August 16, 2022 18:32
@feng-tao
Copy link
Member

amazing! any perf number before vs after?

@kristenarmes
Copy link
Contributor Author

@feng-tao we've seen great improvements in a few tests I've ran, with having deleted the nodes/relations in between runs:

Task # 1: 67676 statements

Before: Successfully published. Elapsed: 203 seconds

After: Successfully published. Elapsed: 8 seconds

Task # 2: 237734 statements

Before: Successfully published. Elapsed: 876 seconds

After: Successfully published. Elapsed: 29 seconds

@feng-tao
Copy link
Member

nice, it would be great if you could share the change to the slack so that others could leverage :) (I still remembered the old days to take long time to update in the etl job)

@kristenarmes kristenarmes mentioned this pull request Feb 1, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkunitsk dkunitsk dkunitsk approved these changes

@feng-tao feng-tao Awaiting requested review from feng-tao feng-tao is a code owner

@jinhyukchang jinhyukchang Awaiting requested review from jinhyukchang jinhyukchang is a code owner

@allisonsuarez allisonsuarez Awaiting requested review from allisonsuarez allisonsuarez is a code owner

@dorianj dorianj Awaiting requested review from dorianj

@verdan verdan Awaiting requested review from verdan verdan is a code owner

@mgorsk1 mgorsk1 Awaiting requested review from mgorsk1 mgorsk1 is a code owner

@youngyjd youngyjd Awaiting requested review from youngyjd youngyjd is a code owner

@dechoma dechoma Awaiting requested review from dechoma dechoma is a code owner

@sewardgw sewardgw Awaiting requested review from sewardgw sewardgw is a code owner

Assignees
No one assigned
Labels
area:databuilder From databuilder folder
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kristenarmes @ozandogrultan @feng-tao @dkunitsk