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.

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

Aurel/persist-graph: Mutate the graph in the background thread #1025

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

naure
Copy link
Collaborator

@naure naure commented Feb 14, 2025

On top of #1023.

  • Put each eye side into its own graph (with column graph_id).
    • Test SQL migrations (test_graph_migrations).
  • Load the graph on startup (method load_graph_store).
    • Test of the loading (test_graph_load).
  • Minimize code in server_hawk.rs.
  • Consolidate with the iris store (same schema, same migrations scripts).
  • Consolidate update of graph and iris store (same tx in server_hawk.rs).

@naure naure requested review from dkales and iliailia February 14, 2025 10:23
Base automatically changed from aurel/server_hawk to main February 14, 2025 14:37
@iliailia
Copy link
Collaborator

Could you merge it with main to have a better view of relevant changes?

@naure naure force-pushed the aurel/persist-graph branch from add08c6 to e5080b0 Compare February 17, 2025 10:15
@naure
Copy link
Collaborator Author

naure commented Feb 17, 2025

Could you merge it with main to have a better view of relevant changes?

Done.

Comment on lines +67 to +68
/// Actor-specific data (e.g. graph mutations).
pub actor_data: A,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this looks fine, just a design question:
Should we aim for the db insertion stuff to also be internal to the actor? This again would now add code that is unique to the CPU impl to the server_hawk.rs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point to try to minimize change in server*.rs. As it stands the GraphStore needs a plain old data structure from the Hawk search, but a complex interaction with the startup, database update, and crash recovery mechanisms.

Copy link
Collaborator

@iliailia iliailia left a comment

Choose a reason for hiding this comment

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

Looks good to me so far.

@iliailia iliailia self-requested a review February 18, 2025 08:02
@naure naure marked this pull request as ready for review February 18, 2025 13:35
@naure naure marked this pull request as draft February 18, 2025 13:44
@naure
Copy link
Collaborator Author

naure commented Feb 18, 2025

  • Consolidate with the iris store (same schema).
  • Consolidate update of graph and iris store (same tx).

Any opinions about this?

@iliailia
Copy link
Collaborator

  • Consolidate with the iris store (same schema).
  • Consolidate update of graph and iris store (same tx).

Any opinions about this?

Sounds like a good idea to me. It's a bit annoying to have both a graph and an iris store as arguments for any HNSW searcher method. Plus, semantically these two objects must relate to each other anyway.

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