feat(trees): batch reparent children during merge#8031
Open
grantfitzsimmons wants to merge 8 commits intomainfrom
Open
feat(trees): batch reparent children during merge#8031grantfitzsimmons wants to merge 8 commits intomainfrom
grantfitzsimmons wants to merge 8 commits intomainfrom
Conversation
Suggested by DeepSeek V4, see below: --- Avoid per-child saves when merging tree nodes by collecting children to reparent and moving them in a single batch. Adds _batch_reparent_children which bulk-updates parent_id, renumbers the tree, runs set_fullnames and validate_tree_numbering once, and refreshes in-memory nodenumber fields for the moved children. This significantly reduces O(N) per-child overhead and improves performance for large trees. The merge() function now accumulates children and delegates to the batch reparent helper.
Add a new TestBatchReparent test suite in test_merge.py: import _batch_reparent_children, validate_tree_numbering and set_fullnames and cover many scenarios for batch reparenting (single/multiple counties, counties with subtrees, fullname updates, nodenumber uniqueness, empty input no-op, mixed reparenting, integration with merge that moves localities, and ordering preservation). These tests exercise tree integrity after bulk reparent operations.
📝 WalkthroughWalkthroughmerge now defers per-child reparenting and batches unmatched children into a new ChangesBatch reparent + merge tests
Sequence Diagram(s)sequenceDiagram
actor Client
participant Merge as extras.merge
participant Batch as _batch_reparent_children
participant DB as Database
participant Renumber as renumber_tree
participant Fullname as set_fullnames
participant Validator as validate_tree_numbering
Client->>Merge: request merge(node, target)
Merge->>DB: SELECT ... FOR UPDATE target
Merge->>Batch: collect unmatched children
Batch->>DB: bulk UPDATE children SET parent_id=target.id
Batch->>Renumber: renumber_tree(collection)
Renumber->>DB: UPDATE nodenumber/highestchildnodenumber (single pass)
Batch->>Fullname: set_fullnames(target_definition)
Fullname->>DB: single UPDATE recomputing fullnames
Batch->>Validator: validate_tree_numbering(collection)
Validator->>DB: read table, run integrity checks
Batch->>Merge: refresh moved children in memory
Merge->>Client: return merge result
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
specifyweb/backend/trees/extras.py (1)
394-436: Consider an early return for empty input to skip unnecessary tree operations.When
childrenis empty, the function still performsrenumber_tree,set_fullnames, andvalidate_tree_numberingon the entire tree. While the behavior is correct, these operations are expensive and unnecessary for a no-op.♻️ Proposed fix
def _batch_reparent_children(children, target, model): + if not children: + return + """Batch-reparent multiple children to a new parent, avoiding O(N) per-child overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specifyweb/backend/trees/extras.py` around lines 394 - 436, The function _batch_reparent_children currently performs expensive tree-wide operations even when the children list is empty; add an early-return guard at the top (e.g., if not children: return) before computing child_ids or calling renumber_tree, set_fullnames, and validate_tree_numbering so we skip the heavy work for a no-op; keep the existing logging behavior but optionally log and return immediately when children is empty to avoid unnecessary DB updates and tree renumbering.specifyweb/backend/trees/tests/test_tree_extras/test_merge.py (2)
4-4: Unused import:set_fullnames.The
set_fullnamesfunction is imported but not used anywhere in this test file.♻️ Proposed fix
-from specifyweb.backend.trees.extras import merge, _batch_reparent_children, validate_tree_numbering, set_fullnames +from specifyweb.backend.trees.extras import merge, _batch_reparent_children, validate_tree_numbering🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specifyweb/backend/trees/tests/test_tree_extras/test_merge.py` at line 4, The import list in the test file includes an unused symbol `set_fullnames`; update the import statement that currently imports `merge, _batch_reparent_children, validate_tree_numbering, set_fullnames` to remove `set_fullnames` (leaving only the actually used symbols) so the unused import is eliminated and linting/tests won’t complain.
165-169: Minor duplication:_make_localityis defined in bothTestMergeandTestBatchReparent.Consider extracting this helper to the
GeographyTreebase class to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specifyweb/backend/trees/tests/test_tree_extras/test_merge.py` around lines 165 - 169, The helper method _make_locality is duplicated in TestMerge and TestBatchReparent; move this method into the common test base GeographyTree so both test classes can inherit it: remove the _make_locality definitions from TestMerge and TestBatchReparent and add a single _make_locality(self, geo) implementation on GeographyTree that returns Locality.objects.create(discipline=self.discipline, geography=geo) so all tests reuse the same helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@specifyweb/backend/trees/extras.py`:
- Around line 394-436: The function _batch_reparent_children currently performs
expensive tree-wide operations even when the children list is empty; add an
early-return guard at the top (e.g., if not children: return) before computing
child_ids or calling renumber_tree, set_fullnames, and validate_tree_numbering
so we skip the heavy work for a no-op; keep the existing logging behavior but
optionally log and return immediately when children is empty to avoid
unnecessary DB updates and tree renumbering.
In `@specifyweb/backend/trees/tests/test_tree_extras/test_merge.py`:
- Line 4: The import list in the test file includes an unused symbol
`set_fullnames`; update the import statement that currently imports `merge,
_batch_reparent_children, validate_tree_numbering, set_fullnames` to remove
`set_fullnames` (leaving only the actually used symbols) so the unused import is
eliminated and linting/tests won’t complain.
- Around line 165-169: The helper method _make_locality is duplicated in
TestMerge and TestBatchReparent; move this method into the common test base
GeographyTree so both test classes can inherit it: remove the _make_locality
definitions from TestMerge and TestBatchReparent and add a single
_make_locality(self, geo) implementation on GeographyTree that returns
Locality.objects.create(discipline=self.discipline, geography=geo) so all tests
reuse the same helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 87e00c71-1a6e-417d-a708-e64cd46d0ff7
📒 Files selected for processing (2)
specifyweb/backend/trees/extras.pyspecifyweb/backend/trees/tests/test_tree_extras/test_merge.py
Deduplicate test helper by moving _make_locality into the GeographyTree base class in tests/test_trees.py and removing duplicate implementations from tests/test_tree_extras/test_merge.py. Also tidy imports in test_merge (removed unused set_fullnames import) and update the helper to reference models.Locality. This centralizes locality creation for geography-related tests without changing test behavior.
Avoids further processing when there's nothing to reparent, preventing unnecessary work.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specifyweb/backend/trees/extras.py`:
- Around line 423-425: renumber_tree currently rebuilds nodenumber from
path_expr(...) which orders siblings by ID and can reorder children compared to
the interval order produced by moving_node; change renumber_tree (or the
path_expr ordering it uses) to perform a stable sort that preserves existing
preorder by using the current nodenumber as a secondary tie-breaker (e.g. order
by path keys then existing nodenumber) so siblings keep their prior interval
order unless explicitly moved; refer to renumber_tree, path_expr(...) and
moving_node for where to apply the stable-sort/tie-break change.
In `@specifyweb/backend/trees/tests/test_tree_extras/test_merge.py`:
- Around line 302-323: The test_batch_reparent_preserves_ordering currently only
checks interval containment; update test_batch_reparent_preserves_ordering to
assert actual sibling ordering after calling _batch_reparent_children by
refreshing and fetching Ohio's children ordered by nodenumber and verifying that
boone, jasper, platte appear in that exact relative order (and optionally
contiguous) among Ohio.children; use the boone/jasper/platte object identities
(or ids) and Geography context to locate them and assert their index/order
positions to prevent sibling reorder regressions.
- Around line 206-220: The test test_batch_reparent_updates_fullnames is
currently using the Geography fixture (which has isinfullname=False) so it won't
detect missing fullname recomputation; update the test to use a node type whose
fullname includes ancestry or assert on a tree whose fullname must change after
reparenting (e.g., use a County/Place fixture or any node with
isinfullname=True) and call/verify _batch_reparent_children(self.greene,
self.ohio, <that type>) then refresh the affected nodes and assert their
fullname reflects the new parent path (reference test name
test_batch_reparent_updates_fullnames, the helper _batch_reparent_children, and
the Geography/County/Place tree type identifiers to locate and modify the test).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86c320f1-a40a-4634-b8ba-096818fa8da1
📒 Files selected for processing (3)
specifyweb/backend/trees/extras.pyspecifyweb/backend/trees/tests/test_tree_extras/test_merge.pyspecifyweb/backend/trees/tests/test_trees.py
✅ Files skipped from review due to trivial changes (1)
- specifyweb/backend/trees/tests/test_trees.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
There was a problem hiding this comment.
♻️ Duplicate comments (1)
specifyweb/backend/trees/tests/test_tree_extras/test_merge.py (1)
206-249:⚠️ Potential issue | 🟠 Major
test_batch_reparent_updates_fullnamesstill won’t fail if fullname recomputation is skippedThis test currently asserts the same values before/after reparenting (
Canislupus,Canislatrans), so a missingset_fullnames()call can still pass undetected.Suggested test adjustment
- # Reparent the Genus Canis from Animalia to Plantae - _batch_reparent_children([canis], plantae, Taxon) + # Create another genus and reparent one species so fullname must change + felis = self.make_taxontree("Felis", "Genus", parent=plantae) + _batch_reparent_children([canis_lupus], felis, Taxon) - # Refresh and check Species fullnames still include Canis + # Refresh and verify fullname changed to new genus canis_lupus.refresh_from_db() canis_latrans.refresh_from_db() - self.assertEqual("Canislupus", canis_lupus.fullname) + self.assertEqual("Felislupus", canis_lupus.fullname) self.assertEqual("Canislatrans", canis_latrans.fullname)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2405338f-275f-41c9-84e7-d9a7aba1455e
📒 Files selected for processing (2)
specifyweb/backend/trees/extras.pyspecifyweb/backend/trees/tests/test_tree_extras/test_merge.py
🚧 Files skipped from review as they are similar to previous changes (1)
- specifyweb/backend/trees/extras.py
grantfitzsimmons
added
2 - Trees
CarolineDenis
approved these changes
May 4, 2026
Contributor
|
I think that we should update the default timeout. |
Raise several timeout settings to 600s to accommodate large uploads and long-running requests. client_body_timeout changed from 120 to 600s, and proxy_connect_timeout, proxy_send_timeout, proxy_read_timeout, and send_timeout were added (all 600s) to prevent premature connection drops during slow client uploads or upstream responses. Existing large body settings (client_max_body_size/client_body_buffer_size) remain unchanged.
Member
Author
|
@CarolineDenis Done in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nginx.conf (1)
32-36: Non-blocking follow-up: monitor connection pressure after raising all timeouts to 600s.Long-lived requests can increase concurrent connection occupancy. Consider tracking nginx active connections plus 499/504 rates after rollout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nginx.conf` around lines 32 - 36, You increased all request timeouts (client_body_timeout, proxy_connect_timeout, proxy_send_timeout, proxy_read_timeout, send_timeout) to 600s which can raise concurrent connection pressure; add monitoring and alerts for nginx active connections and upstream error rates (HTTP 499 and 504) after rollout by instrumenting nginx metrics (stub_status or nginx-vts) and your metrics backend, create dashboards for active/concurrent connections and 499/504 rates, and add threshold-based alerts to detect sustained increases so you can roll back or tune timeouts if connection saturation or elevated 499/504s occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@nginx.conf`:
- Around line 32-36: You increased all request timeouts (client_body_timeout,
proxy_connect_timeout, proxy_send_timeout, proxy_read_timeout, send_timeout) to
600s which can raise concurrent connection pressure; add monitoring and alerts
for nginx active connections and upstream error rates (HTTP 499 and 504) after
rollout by instrumenting nginx metrics (stub_status or nginx-vts) and your
metrics backend, create dashboards for active/concurrent connections and 499/504
rates, and add threshold-based alerts to detect sustained increases so you can
roll back or tune timeouts if connection saturation or elevated 499/504s occur.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8029
Code was suggested by DeepSeek V4 🤖, reviewed by me, so see below:
Before:
Merging would fail or timeout in most cases. See #8029 for full details.
After:
Merging succeeds without issue.
mergingAndrenidae.mp4
Checklist
self-explanatory (or properly documented)
Testing instructions
Developers
I tested these on
naturkundemuseum_test_2026_04_27linked in the original issue.Run the existing merge tests to verify no regressions:
Run the new batch reparent tests specifically:
Question: @specify/developers Should we update the default timeout in development and production for Nginx to avoid seeing an error on the front-end despite the process completing in the background? Users might see "504 Gateway Time-out" if this is not adjusted.
I adjusted this:
specify7/nginx.conf
Lines 28 to 32 in bc7ea55
To this:
UX Testing
Note: You may see a "504 Gateway Time-out" error if Nginx is configured to have a short timeout. Even if this appears, make sure the action finishes behind-the-scenes.
Manual testing on a real database (e.g.,
naturkundemuseum_test_2026_04_27):Andrenidaenodes that were previously failingPerformance verification (optional):
Summary by CodeRabbit
Refactor
Bug Fixes
Chores