perf(cascade): fix bulk-insert-with-TTL regression (v1.3.4) - #288
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesCascade-aware TTL refresh
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant init_rapyer
participant AtomicRedisModel
participant cascade_planner
participant RedisPipeline
init_rapyer->>cascade_planner: build_cascade_plan()
init_rapyer->>AtomicRedisModel: cache _has_cascade_edges
AtomicRedisModel->>cascade_planner: _cascade_edges_present()
cascade_planner-->>AtomicRedisModel: cascade edge presence
AtomicRedisModel->>RedisPipeline: expire model keys when no cascade edges
AtomicRedisModel->>RedisPipeline: run cascade FCALL when cascade edges exist
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Coverage reportTotal coverage: 99% Full report |
Merging this PR will improve performance by 12.45%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rapyer/base.py (1)
618-619: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for
aset_ttl’s native fast path.The supplied tests cover
refresh_ttl, butaset_ttlhas distinct execution and return behavior forcascade=False,cascade=True, and outer pipelines. Add no-edge cases verifying expiration ofall_keys, no cascade FCALL, and the documented return values.As per path instructions,
tests/**should focus on test coverage completeness and edge cases.🤖 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 `@rapyer/base.py` around lines 618 - 619, Add regression tests for aset_ttl’s native fast path when no cascade edges exist, covering cascade=False, cascade=True, and execution inside an outer pipeline. Assert all_keys expiration, confirm no cascade FCALL occurs, and verify each documented return value while preserving the existing refresh_ttl coverage.Source: Path instructions
🤖 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 `@rapyer/base.py`:
- Around line 618-619: Add regression tests for aset_ttl’s native fast path when
no cascade edges exist, covering cascade=False, cascade=True, and execution
inside an outer pipeline. Assert all_keys expiration, confirm no cascade FCALL
occurs, and verify each documented return value while preserving the existing
refresh_ttl coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 949bb3fa-3b74-4cc5-8aa5-4e73e109308e
📒 Files selected for processing (5)
rapyer/base.pyrapyer/cascade/planner.pyrapyer/init.pytests/unit/cascade/test_refresh_ttl_cascade_branch.pytests/unit/test_refresh_ttl_if_needed.py
yedidyakfir
force-pushed
the
gsd/ttl-codspeed-regression
branch
from
July 23, 2026 19:28
015502e to
81e9ba8
Compare
yedidyakfir
force-pushed
the
gsd/ttl-codspeed-regression
branch
from
July 24, 2026 09:41
81e9ba8 to
47298dc
Compare
yedidyakfir
force-pushed
the
gsd/ttl-codspeed-regression
branch
from
July 24, 2026 09:41
47298dc to
f99e293
Compare
…egression v1.3.4's cascade-TTL milestone made refresh_ttl/aset_ttl always issue a per-model cascade FCALL on real Redis instead of a plain pipe.expire. Since ainsert auto-refreshes TTL per result model, bulk-inserting N models with no ForeignKey fields paid N heavier FCALLs -- pure overhead -- which regressed the *Many/*MixedClasses _with_ttl benchmarks ~13% while single insert did not. Gate the native-EXPIRE fast path on the _relational_field_names / _contain_fk sets that __init_subclass__ already populates: a model with no FK fields takes the plain pipe.expire path, and any model that has FK fields defers to the cascade function (which no-ops when its plan carries no enabled edges). Genuine cascade roots are unchanged -- still atomic, server-side, cycle/depth guards intact. No new class variable, no cache, no cross-module import cycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
yedidyakfir
force-pushed
the
gsd/ttl-codspeed-regression
branch
from
July 24, 2026 13:07
f99e293 to
e2ac3e2
Compare
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.
Problem
CodSpeed flagged a regression in v1.3.4 (the cascade-TTL milestone): bulk
ainsertwith a TTL got ~13% slower vs v1.3.3, while single insert and ~80 other benchmarks improved.Isolated via CodSpeed compare (v1.3.3
9b3c9af→ v1.3.4ab6ec37): exactly 3 regressions, alltest_benchmark_with_ttlon the multi-insert path:test_model.py::TestModelInsertMany(4.5 → 5.1 ms, −13.2%)test_module_api.py::TestModuleAinsertMany(4.8 → 5.5 ms, −13.1%)test_module_api.py::TestModuleAinsertMixedClasses(4.8 → 5.6 ms, −13.2%)Single insert with TTL did not regress (+11% faster) → a per-model cost that scales with the number of models inserted.
Root cause
v1.3.4 rewrote
refresh_ttl/aset_ttlto always issue a per-model cascade Redis-FunctionFCALLon real Redis instead of a plainpipe.expire. Sinceainsertauto-refreshes TTL per result model, bulk-inserting N no-FK models pays N heavier FCALLs — pure overhead when the models have no cascade edges.Fix
Gate the FCALL on a per-model
_has_cascade_edgesflag (set byinit_rapyerfrom the built cascade plan; lazily resolved otherwise). Models with no cascade-enabled FK edges take the pre-v1.3.4 nativepipe.expire(all_keys)fast path; the FCALL only fires when there is an actual cascade graph to walk. Genuine cascade roots are unchanged — still atomic, server-side, cycle/depth guards intact. Additive; no change toMeta.ttl/refresh_ttlsemantics.Verification
import rapyerOK; affected unit tests 24/24; debugger self-verification reported unit 802 pass, cascade+refresh-ttl 86 pass, integration FK 40 pass, ttl+FK 23 pass, black + ruff clean.test_benchmark_with_ttlregressions clear with no newtest_cascade_ttlregressions.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance
Tests