-
Notifications
You must be signed in to change notification settings - Fork 192
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
✨ NEW: Add Backend
bulk methods
#5171
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5171 +/- ##
===========================================
+ Coverage 80.93% 80.96% +0.04%
===========================================
Files 536 535 -1
Lines 37056 37185 +129
===========================================
+ Hits 29986 30104 +118
- Misses 7070 7081 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Backend.bulk_insert
& Backend.bulk_update
Backend
bulk methods
@@ -78,46 +94,115 @@ def transaction(self): | |||
if session.in_transaction(): | |||
with session.begin_nested(): | |||
yield session | |||
session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems an important change - was it wrong before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was a regression error from moving to the SQLAlchemy v2 API, that I realised in #4534, i.e. it was not actually committing the transaction when exiting the transaction context for archive imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I'm approving, but great if you could double check my comment above.
Also, one more general question, now many PRs have at the bottom a number of "errors" under the title "Unchanged files with check annotations". What are these? Should they be ignored?
Yeh I don't know either, its showing errors when it encounters an exception raise, which seems nonsensical for a static analyser. May be related to actions/toolkit#457 |
Partially extracted from (and required by) #5145 and also work towards #5154
Adds
Backend.bulk_insert
andBackend.bulk_update
methods, and moves thedelete_nodes_and_connections
function to a method on theBackend
.This removes the need for using backend specific code outside of the backend, for these use cases (still plenty more to fix)
delete_nodes_and_connections
performs multiple operations and so should only be called within a transaction, thus it raises anAssertionError
if this is not the case.(this addresses, but maybe not fully, #3382 and #2496)