Conversation
* feat: reorganize generators with base class and mixins * fix: import formatting
…y complete (#8) * feat: add stream_many and stream_batch to get pipeline results as they complete * fix: batch context copy * fix: deep copy context --------- Co-authored-by: Pierre Le Jeune <pierre@giskard.ai>
* Fix: Added missing finish reason * Update src/counterpoint/generators/base.py --------- Co-authored-by: Pierre Le Jeune <pierre@giskard.ai>
* Added temperature * Fix default temperature in pipeline * Moved temperature conf to generator * Ran linter * Fixed typing issue with conftest --------- Co-authored-by: Pierre Le Jeune <pierre@giskard.ai>
* feat: better transcript for chats with tool usage
* feat: add retry policy to generators * fix: remove unused code * fix retry for batch_complete
mattbit
requested changes
Feb 11, 2026
Member
mattbit
left a comment
mattbit
left a comment
There was a problem hiding this comment.
The approach is good, but the implementation has some little bugs/improvements to fix.
I would also rename the module to rate_limiter since this seems to be the term used for all objects.
* improve workflows * add labeler workflow * drop ci scope * pin action versions
- Adjusted dependency grouping settings to match all package names and update types. - Changed group name and slug to be dynamic based on the manager.
…-limiter-global-kind-target-to-giskard-agents
…in BasicRateLimiter This commit improves type annotations for the `RateLimiterRegistry` class by specifying the type for `_lock` and initializing `instances` with a type hint in the `get` method. Additionally, the unused `model_post_init` method in `BasicRateLimiter` has been removed to streamline the code.
…ate limiters This commit improves the state management in the `RateLimiterRegistry` and `BaseRateLimiter` classes by introducing a private attribute for state handling. It also updates type annotations for better clarity and consistency. The unused `get_state` method has been removed, streamlining the code further.
This commit introduces a warning mechanism in the `RateLimiterRegistry` to alert users when attempting to register a rate limiter with an existing ID. A new environment variable, `GISKARD_DISABLE_DUPLICATE_RATE_LIMITERS_WARNINGS`, has been added to control the display of these warnings. Additionally, tests have been updated to verify the warning behavior when duplicate IDs are used, ensuring that the functionality works as intended.
docs(limiter): enhance module docstrings and add method descriptions This commit improves the documentation for the rate limiter modules by adding detailed docstrings to classes and methods. The changes provide clearer explanations of the functionality, parameters, and return values, enhancing the overall understanding of the rate limiting system. """
…l-kind-target-to-giskard-agents
…l-kind-target-to-giskard-agents
Member
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.gh.umua.top/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is a solid pull request that refactors the rate limiting mechanism into the core library, enhancing modularity and reusability. The new rate limiter implementation is robust, featuring a registry for state sharing across instances and comprehensive test coverage. Additionally, the retry policy has been improved with max_delay and timeout support for generator requests, both of which are well-tested. I have one suggestion to improve the robustness of the retry condition logic.
…l-kind-target-to-giskard-agents
…p tests - Make BaseRateLimiter abstract (ABC) with throttle as @AbstractMethod - Add lock to get_instance for consistency with register_instance - Fix docstring referencing wrong class name (RateLimiter → BaseRateLimiter) - Fix RetriableError extending BaseException instead of Exception - Remove dead mock_response fixture with bogus self parameter - Use uuid ids in registry tests to isolate from singleton state - Document rate limiting inside retry loop on LiteLLMGenerator
…patibility Without a yield in the body, Pyright classifies the async def as a regular coroutine rather than an async generator function, causing a type mismatch with @asynccontextmanager. Made-with: Cursor
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
6 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.
Uh oh!
There was an error while loading. Please reload this page.