Skip to content

Batch operation refactoring#297

Merged
JoOkuma merged 11 commits into
royerlab:mainfrom
yfukai:batch_remove
Jun 10, 2026
Merged

Batch operation refactoring#297
JoOkuma merged 11 commits into
royerlab:mainfrom
yfukai:batch_remove

Conversation

@yfukai

@yfukai yfukai commented May 29, 2026

Copy link
Copy Markdown
Contributor

Human summary

  • Adds batch_remove_*
  • Dropping duplicate codes by calling bulk_add_... inside add_...
  • Batch handling of events; replaces Batch handling of events #270

Copilot Summary

This pull request introduces bulk mutation operations and refactors node and edge addition/removal logic across the graph array and graph view classes. The changes improve efficiency, ensure atomic validation, and unify signal emission for both single and bulk operations. The most important updates are summarized below.

Bulk mutation API additions

  • Added bulk_remove_nodes and bulk_remove_edges methods to BaseGraph, RustWorkXGraph, and GraphView, enabling efficient removal of multiple nodes or edges at once with atomic validation and proper signal emission. [1] [2] [3] [4] [5]
  • Introduced low-level local mutation helpers (_bulk_add_nodes_local, _bulk_remove_nodes_local, _bulk_add_edges_local, _bulk_remove_edges_local) in RustWorkXGraph to centralize and optimize node/edge manipulation logic.

Signal emission and event handling improvements

  • Refactored signal emission for node addition and update to use new utility functions (emit_node_added_events, emit_node_updated_events), ensuring consistent and efficient signal handling for both single and bulk operations. [1] [2] [3] [4] [5]
  • Updated the GraphArray event handlers to support bulk node add/update events by iterating over event sequences, improving compatibility with the new bulk APIs. [1] [2]

Codebase and API consistency

  • Refactored single-node add/remove methods to internally use the new bulk primitives, reducing code duplication and ensuring consistent behavior. [1] [2] [3] [4]
  • Improved error handling in ID mapping logic to ensure user-facing errors are consistent between single and bulk operations.

@yfukai yfukai mentioned this pull request May 29, 2026
16 tasks
@codecov-commenter

codecov-commenter commented May 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.14286% with 80 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.64%. Comparing base (13f9ba8) to head (8cf69b8).

Files with missing lines Patch % Lines
src/tracksdata/graph/_graph_view.py 59.64% 12 Missing and 11 partials ⚠️
src/tracksdata/graph/_base_graph.py 8.33% 22 Missing ⚠️
src/tracksdata/utils/_signal.py 66.66% 11 Missing and 5 partials ⚠️
src/tracksdata/graph/_rustworkx_graph.py 93.10% 4 Missing and 4 partials ⚠️
src/tracksdata/graph/filters/_spatial_filter.py 72.00% 4 Missing and 3 partials ⚠️
src/tracksdata/graph/_sql_graph.py 94.11% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
- Coverage   87.81%   86.64%   -1.18%     
==========================================
  Files          57       57              
  Lines        4998     5181     +183     
  Branches      877      924      +47     
==========================================
+ Hits         4389     4489     +100     
- Misses        384      444      +60     
- Partials      225      248      +23     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yfukai yfukai mentioned this pull request May 29, 2026

@yfukai yfukai left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LGTM

@yfukai yfukai marked this pull request as ready for review June 5, 2026 02:18
Comment thread src/tracksdata/utils/_signal.py Outdated

def reduce_node_added_events(
event_args: Iterable[tuple[int, dict[str, Any]]],
) -> tuple[int | list[int], dict[str, Any] | list[dict[str, Any]]]:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yfukai, could you add the documentation to the public functions of this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@JoOkuma JoOkuma left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yfukai, thanks for the PR I really appreciate how you simplified some of the functions.

Comment thread src/tracksdata/graph/_sql_graph.py Outdated
for i in range(0, len(edge_ids), chunk_size):
chunk = edge_ids[i : i + chunk_size]
existing.update(
row[0] for row in session.query(self.Edge.edge_id).filter(self.Edge.edge_id.in_(chunk)).all()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yfukai, is there a way to query the missing edge IDs during the delete query? And if some are missing, it discards the session rather than committing and raises the error.
The goal is to reduce the number of queries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I cleaned up the code!

Comment thread src/tracksdata/graph/_sql_graph.py Outdated
Comment on lines +1044 to +1045
for nid in node_ids:
self.node_removed.emit(nid, old_attrs_per_node[nid])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make sense to have a emit_node_deleted_events?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@yfukai

yfukai commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

I worked on the followings:

  • Unifying signal signature to (list of node_ids, list of attrs) to simplify the codebase.
  • Now the BaseGraph has the concrete functions such as add_node that calls bulk_add_nodes and leaves the latter to each inheriting class.

@yfukai yfukai left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments @JoOkuma! I believe I could follow your suggestions as well as cleaning up the codebase.

Comment thread src/tracksdata/utils/_signal.py Outdated

def reduce_node_added_events(
event_args: Iterable[tuple[int, dict[str, Any]]],
) -> tuple[int | list[int], dict[str, Any] | list[dict[str, Any]]]:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment thread src/tracksdata/graph/_sql_graph.py Outdated
Comment on lines +1044 to +1045
for nid in node_ids:
self.node_removed.emit(nid, old_attrs_per_node[nid])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment thread src/tracksdata/graph/_sql_graph.py Outdated
for i in range(0, len(edge_ids), chunk_size):
chunk = edge_ids[i : i + chunk_size]
existing.update(
row[0] for row in session.query(self.Edge.edge_id).filter(self.Edge.edge_id.in_(chunk)).all()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I cleaned up the code!

@JoOkuma

JoOkuma commented Jun 10, 2026

Copy link
Copy Markdown
Member

Thanks @yfukai

@JoOkuma JoOkuma merged commit 3ee2603 into royerlab:main Jun 10, 2026
7 checks passed
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