-
-
Notifications
You must be signed in to change notification settings - Fork 5
fix: padacioso speed #33
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
Conversation
|
Warning Rate limit exceeded@mikejgray has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
padacioso/__init__.py (1)
280-280: Unnecessary cache invalidation in context/keyword operations.
exclude_keywords,exclude_context,unexclude_context,require_context, andunrequire_contextall mark_cache_dirty = True, but the cache (_intent_list) only contains data fromintent_samples. Context and keyword operations don't modifyintent_samples, so cache invalidation is unnecessary here.Remove cache invalidation from these methods (lines 280, 292, 297, 309, 314):
def exclude_keywords(self, intent_name, samples): if intent_name not in self.excluded_keywords: self.excluded_keywords[intent_name] = samples else: self.excluded_keywords[intent_name] += samples - self._cache_dirty = True # Mark cache as needing rebuildApply similar changes to
exclude_context(line 292),unexclude_context(line 297),require_context(line 309), andunrequire_context(line 314).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/unit_tests.yml(1 hunks).gitignore(1 hunks)padacioso/__init__.py(7 hunks)
🔇 Additional comments (4)
.github/workflows/unit_tests.yml (1)
16-23: LGTM! CI workflow updates are appropriate.The Python version matrix update (3.9-3.13) and GitHub Actions version bumps (checkout v4, setup-python v5) are sensible modernization changes.
padacioso/__init__.py (3)
39-41: LGTM! Cache initialization is well-designed.The lazy cache pattern with
_intent_listand_cache_dirtyflag is a sound optimization to avoid repeated list construction on every query.
98-98: LGTM! Cache invalidation on intent removal is correct.Marking the cache dirty when an intent is removed ensures the cached list stays synchronized with
intent_samples.
124-131: LGTM! Cache rebuild implementation is straightforward.The lazy rebuild approach (only rebuilding when dirty) avoids O(n²) overhead during bulk intent registration.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Refactor
Chores