Refactor graph persistence structure and update CLI command syntax#29
Refactor graph persistence structure and update CLI command syntax#29Orio77 wants to merge 2 commits into
Conversation
…I commands - Made persistence logic more robust - Moved graph dump functions to a new persistence module for better structure. - Updated CLI commands to reflect new module paths. - Enhanced .env.example with additional comments for clarity on Neo4j import paths.
- Changed 'prefect_pipeline' to 'prefect-pipeline' in multiple documentation files to ensure uniformity in command usage.
Irahan2
left a comment
Irahan2
left a comment
There was a problem hiding this comment.
Nice direction overall. I left a few suggestions.
| # Optional. Path *inside Neo4j’s import folder* for APOC export/import (not your repo path). | ||
| # Unset = graph_export.cypher at import root (typical for Neo4j Desktop). | ||
| # Docker stack: use dumps/graph_export.cypher so it matches import/dumps -> ../dumps in compose. | ||
| # NEO4J_IMPORT_REL_PATH=dumps/graph_export.cypher |
There was a problem hiding this comment.
I think this should not stay commented for the Docker setup, or Docker needs a different default. with the current default, the code writes graph_export.cypher to Neo4j import root, but Docker only mounts ../dumps to /var/lib/neo4j/import/dumps. So dump-graph may finish, but the dump will not appear in the repo dumps/ folder. Maybe we should set this in Docker env by default, or make the default path Docker-safe and document the Desktop override.
|
|
||
| def _copy_dump_to_repo_if_needed(out: Path, rel: str) -> None: | ||
| """If APOC wrote under Neo4j's import tree, copy the file into the repo ``dumps/`` path.""" | ||
| if out.is_file(): |
There was a problem hiding this comment.
This can leave an old dump in place. If dumps/graph_export.cypher already exists, we return here and never copy the newly exported file from Neo4j's import dir. So after a fresh export, the repo dump can still be stale.
I think we should overwrite/copy after every successful export, or at least compare the source file and timestamp before skipping.
|
|
||
|
|
||
| def _import_directory_from_server(session) -> Path | None: | ||
| """Resolve the DBMS import directory (absolute on the server host; same as this machine for Desktop).""" |
There was a problem hiding this comment.
Okay in here small CI thing: uv run ruff check src fails here and also ı beleive around lines 127 and 194 because these docstrings are over the 100 char limit. Could you wrap these lines so the lint check passes?
2 participants
Rozwiązane błędy ścieżek przy zapisie, mała reorganizacja, poprawiony syntax komendy pipeline
This pull request refactors and enhances the Neo4j graph dump and restore functionality in the data pipeline. It moves related code into a new
persistencemodule, improves configuration flexibility, and makes the import/export process more robust and compatible across environments (including Neo4j Desktop and Docker). It also updates documentation and scripts to reflect these changes.Refactoring and code organization:
src/data_pipeline/graph_dump.pyto a new modulesrc/data_pipeline/persistence/graph_dump.py, and updated all imports and CLI entrypoints accordingly. The CLI code was moved fromsrc/data_pipeline/cli.pytosrc/data_pipeline/persistence/cli.py. [1] [2] [3]Improvements to graph dump/restore process:
_neo4j_import_rel_path,_project_root, and_copy_dump_to_repo_if_needed. Added a check for non-empty dump files before restoring (host_nonempty_dump_exists).apoc.cypher.runManyfor better compatibility, and improved error handling for missing APOC procedures.Configuration and documentation updates:
.env.examplewith clearer explanations and new options for dump file paths, import directory, and backup locations. [1] [2]CLAUDE.md, debugging guides) to use the newprefect-pipelinecommand and reflect the new module structure. [1] [2]Testing improvements:
host_nonempty_dump_existshelper for mocking dump file existence.