fix(backend): create node with many rels faster #6883
Conversation
WalkthroughThe asynchronous method Changes
Estimated code review effort3 (~45 minutes) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
📓 Path-based instructions (2)backend/tests/**/*📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Files:
backend/**/*📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Files:
🧬 Code Graph Analysis (4)backend/tests/helpers/constants.py (1)
backend/tests/unit/core/migrations/graph/test_012.py (1)
backend/infrahub/core/relationship/model.py (4)
backend/tests/unit/core/migrations/graph/test_013.py (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)backend/tests/**/*📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Files:
backend/**/*📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Files:
🧬 Code Graph Analysis (4)backend/tests/helpers/constants.py (1)
backend/tests/unit/core/migrations/graph/test_012.py (1)
backend/infrahub/core/relationship/model.py (4)
backend/tests/unit/core/migrations/graph/test_013.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (16)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
group/backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/infrahub/core/query/node.py (1)
162-162: Approve the optimization with a suggestion for defensive programming.The peer assignment logic is correct and maintains the same functionality while improving performance. However, consider adding error handling for the case where
rel.peer_idmight not exist in thepeersdictionary.Consider this defensive approach:
- await rel.set_peer(value=peers[rel.peer_id]) + if rel.peer_id in peers: + await rel.set_peer(value=peers[rel.peer_id]) + else: + # Log warning or handle missing peer case + raise ValueError(f"Peer ID {rel.peer_id} not found in fetched peers")Alternatively, if you're confident that
rel.peer_idwill always exist inpeers, the current implementation is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/infrahub/core/query/node.py(1 hunks)
📓 Path-based instructions (1)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/query/node.py
🧰 Additional context used
📓 Path-based instructions (1)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/query/node.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E-testing-version-upgrade / From 1.1.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-invoke-demo-start
- GitHub Check: backend-docker-integration
- GitHub Check: E2E-testing-playwright
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-tests-unit
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
backend/infrahub/core/query/node.py (1)
160-160: Excellent optimization approach!Fetching all peers at once before the relationship processing loop is a smart performance improvement that follows the batch operation pattern to reduce database calls.
fatih-acar
force-pushed
the
fac-faster-many-rels-create
branch
from
82f7e2f to
92acc57
Compare
CodSpeed Performance ReportMerging #6883 will degrade performances by 51.75%Comparing Summary
Benchmarks breakdown
|
Avoid fetching each peer during node creation by fetching them all at once prior to calling get_create_data(). Signed-off-by: Fatih Acar <[email protected]>
fatih-acar
force-pushed
the
fac-faster-many-rels-create
branch
from
92acc57 to
4ad348f
Compare
backend/infrahub/core/query/node.py
Outdated
| peers = await rel_manager.get_peers(db=db, branch_agnostic=self.branch_agnostic) | ||
| for rel in rel_manager._relationships: | ||
| if rel.get_peer_id() in peers: | ||
| await rel.set_peer(value=peers[rel.get_peer_id()]) |
There was a problem hiding this comment.
| peers = await rel_manager.get_peers(db=db, branch_agnostic=self.branch_agnostic) | |
| for rel in rel_manager._relationships: | |
| if rel.get_peer_id() in peers: | |
| await rel.set_peer(value=peers[rel.get_peer_id()]) | |
| for rel in rel_manager.get_relationships(db=db, branch_agnostic=self.branch_agnostic): |
This should fetch all relationships at once, and it avoids accessing private members _relationships
There was a problem hiding this comment.
I already tried that and the thing is that get_relationships() only sets the peer ids (and does not call a get_peers equivalent to fetch the full peer node).
I wonder if we really need the peer attributes for the NodeCreateAll query to work, maybe we can just avoid the peer.resolve() call within the rel.get_create_data function.
There was a problem hiding this comment.
I already tried that and the thing is that get_relationships() only sets the peer ids (and does not call a get_peers equivalent to fetch the full peer node).
Right
It seems we need to fetch to get the branch peer
…ests with missing core schemas
Avoid fetching each peer during node creation by fetching them all at once prior to calling get_create_data().
Summary by CodeRabbit