LCORE-741: Proper quota limiters configuration #679
Conversation
WalkthroughAdds QuotaLimiterConfiguration and QuotaSchedulerConfiguration, renames and extends QuotaHandlersConfig → QuotaHandlersConfiguration (adds limiters and scheduler), updates Configuration.quota_handlers default and Pydantic imports, and adds tests plus UML doc updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/models/config.py (1)
578-582: Consider extending ConfigurationBase for consistency.
QuotaSchedulerConfigurationextendsBaseModelinstead ofConfigurationBase, which means it won't reject unknown fields (noextra="forbid"). This is inconsistent with other configuration models in the codebase and violates the coding guideline that states: "All configuration uses Pydantic models extending ConfigurationBase" and "Configuration base models must set model_config with extra='forbid' to reject unknown fields."As per coding guidelines
Apply this diff to maintain consistency:
-class QuotaSchedulerConfiguration(BaseModel): +class QuotaSchedulerConfiguration(ConfigurationBase): """Quota scheduler configuration.""" period: PositiveInt = 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/models/config.py(3 hunks)tests/unit/models/config/test_dump_configuration.py(3 hunks)tests/unit/models/config/test_quota_handlers_config.py(1 hunks)tests/unit/models/config/test_quota_limiter_config.py(1 hunks)tests/unit/models/config/test_quota_scheduler_config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/models/config/test_quota_handlers_config.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_quota_scheduler_config.pytests/unit/models/config/test_quota_limiter_config.pysrc/models/config.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/models/config/test_quota_handlers_config.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_quota_scheduler_config.pytests/unit/models/config/test_quota_limiter_config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/models/config/test_quota_handlers_config.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_quota_scheduler_config.pytests/unit/models/config/test_quota_limiter_config.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/config.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/models/config.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/config.py
🧬 Code graph analysis (4)
tests/unit/models/config/test_quota_handlers_config.py (1)
src/models/config.py (3)
config(140-146)QuotaHandlersConfiguration(584-593)QuotaSchedulerConfiguration(578-581)
tests/unit/models/config/test_dump_configuration.py (1)
src/models/config.py (13)
QuotaHandlersConfiguration(584-593)QuotaLimiterConfiguration(568-575)QuotaSchedulerConfiguration(578-581)Configuration(596-622)ServiceConfiguration(149-166)TLSConfiguration(38-48)CORSConfiguration(51-72)LlamaStackConfiguration(177-220)UserDataCollection(223-256)DatabaseConfiguration(108-146)PostgreSQLDatabaseConfiguration(87-105)InferenceConfiguration(494-511)dump(619-622)
tests/unit/models/config/test_quota_scheduler_config.py (1)
src/models/config.py (2)
config(140-146)QuotaSchedulerConfiguration(578-581)
tests/unit/models/config/test_quota_limiter_config.py (1)
src/models/config.py (2)
config(140-146)QuotaLimiterConfiguration(568-575)
🪛 GitHub Actions: Python linter
tests/unit/models/config/test_quota_handlers_config.py
[error] 3-3: Pylint: Unused import 'pytest' (unused-import).
tests/unit/models/config/test_quota_scheduler_config.py
[error] 3-3: Pylint: Unused import 'pytest' (unused-import).
🪛 GitHub Actions: Ruff
tests/unit/models/config/test_quota_handlers_config.py
[error] 3-3: F401: 'pytest' imported but unused
tests/unit/models/config/test_quota_scheduler_config.py
[error] 3-3: F401: 'pytest' imported but unused
⏰ 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). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (9)
tests/unit/models/config/test_quota_limiter_config.py (2)
8-22: LGTM!The test comprehensively validates QuotaLimiterConfiguration with all required fields and appropriate assertions.
25-60: LGTM!The validation tests properly verify NonNegativeInt constraints and Literal type restrictions using pytest.raises with appropriate error message matching.
src/models/config.py (4)
20-20: LGTM!The
NonNegativeIntimport is correctly added and used inQuotaLimiterConfigurationforinitial_quotaandquota_increasefields.
568-576: LGTM!The
QuotaLimiterConfigurationclass is well-structured with appropriate field types and validation constraints. The use ofNonNegativeIntfor quota fields ensures valid values.
584-593: LGTM!The
QuotaHandlersConfigurationupdates correctly integrate the newlimiterslist andschedulerfields with appropriate default factories.
615-617: LGTM!The
quota_handlersfield is properly configured with a default factory forQuotaHandlersConfiguration.tests/unit/models/config/test_dump_configuration.py (3)
16-18: LGTM!The new imports for quota-related configuration models are correctly added and used in the new test function.
181-182: LGTM!The assertions correctly validate the default serialization of an empty
limiterslist and aschedulerwith the defaultperiodvalue of 1.
303-498: LGTM!The new test function comprehensively validates serialization of quota limiters configuration, including:
- Multiple limiters with different types (user_limiter, cluster_limiter)
- Custom scheduler period
- Token history flag
- Complete JSON structure verification
The test ensures that all quota-related fields are correctly serialized and deserialized.
tisnik
force-pushed
the
lcore-741-quota-limiters-configuration
branch
from
cd756dc to
10b81fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/unit/models/config/test_quota_scheduler_config.py (2)
6-11: Consider removing redundant assertion.The
assert cfg is not Noneon line 9 is redundant since Pydantic model instantiation will raise an exception if construction fails. The meaningful assertion is on line 11.Apply this diff:
def test_quota_scheduler_default_configuration() -> None: """Test the default configuration.""" cfg = QuotaSchedulerConfiguration() - assert cfg is not None # default value assert cfg.period == 1
14-18: Consider removing redundant assertion and adding edge-case tests.The
assert cfg is not Noneon line 17 is redundant for the same reason as in the first test.Additionally, consider adding tests for validation edge cases (e.g., zero, negative values) to ensure
PositiveIntvalidation works as expected.Apply this diff for the redundant assertion:
def test_quota_scheduler_custom_configuration() -> None: """Test the custom configuration.""" cfg = QuotaSchedulerConfiguration(period=10) - assert cfg is not None assert cfg.period == 10Optionally add validation tests:
def test_quota_scheduler_validation_rejects_zero() -> None: """Test that period validation rejects zero.""" with pytest.raises(ValidationError): QuotaSchedulerConfiguration(period=0) def test_quota_scheduler_validation_rejects_negative() -> None: """Test that period validation rejects negative values.""" with pytest.raises(ValidationError): QuotaSchedulerConfiguration(period=-1)tests/unit/models/config/test_quota_handlers_config.py (1)
6-20: Consider removing redundant assertion and verifying scheduler configuration.Two suggestions:
The
assert cfg is not Noneon line 15 is redundant since Pydantic model instantiation will raise an exception if construction fails.Line 19 checks that
scheduleris not None but doesn't verify that the customperiod=10was actually set. Consider addingassert cfg.scheduler.period == 10to validate the custom scheduler configuration.Apply this diff:
def test_quota_handlers_configuration() -> None: """Test the quota handlers configuration.""" cfg = QuotaHandlersConfiguration( sqlite=None, postgres=None, limiters=[], scheduler=QuotaSchedulerConfiguration(period=10), enable_token_history=False, ) - assert cfg is not None assert cfg.sqlite is None assert cfg.postgres is None assert cfg.limiters == [] assert cfg.scheduler is not None + assert cfg.scheduler.period == 10 assert not cfg.enable_token_history
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/config.pngis excluded by!**/*.pngdocs/config.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
docs/config.puml(3 hunks)tests/unit/models/config/test_quota_handlers_config.py(1 hunks)tests/unit/models/config/test_quota_scheduler_config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/models/config/test_quota_handlers_config.pytests/unit/models/config/test_quota_scheduler_config.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/models/config/test_quota_handlers_config.pytests/unit/models/config/test_quota_scheduler_config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/models/config/test_quota_handlers_config.pytests/unit/models/config/test_quota_scheduler_config.py
🧠 Learnings (2)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/{models/config.py,configuration.py} : All configuration uses Pydantic models extending ConfigurationBase
Applied to files:
docs/config.puml
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/models/**/*.py : Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Applied to files:
docs/config.puml
🧬 Code graph analysis (2)
tests/unit/models/config/test_quota_handlers_config.py (1)
src/models/config.py (3)
config(140-146)QuotaHandlersConfiguration(584-593)QuotaSchedulerConfiguration(578-581)
tests/unit/models/config/test_quota_scheduler_config.py (1)
src/models/config.py (2)
config(140-146)QuotaSchedulerConfiguration(578-581)
⏰ 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). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (1)
docs/config.puml (1)
148-148: Verify the period field type inconsistency.The
periodfield has different types inQuotaLimiterConfiguration(str) andQuotaSchedulerConfiguration(Annotated). Please confirm whether this difference is intentional due to different validation requirements, or if both should use the same type for consistency.Also applies to: 153-153
| class "QuotaHandlersConfiguration" as src.models.config.QuotaHandlersConfiguration { | ||
| enable_token_history : bool | ||
| limiters : Optional[list[QuotaLimiterConfiguration]] | ||
| postgres : Optional[PostgreSQLDatabaseConfiguration] | ||
| scheduler : Optional[QuotaSchedulerConfiguration] | ||
| sqlite : Optional[SQLiteDatabaseConfiguration] | ||
| } |
There was a problem hiding this comment.
Missing composition declarations for limiters and scheduler fields.
The QuotaHandlersConfiguration class contains two composition fields (limiters and scheduler), but the corresponding composition relationship declarations are missing from the diagram.
Add the missing composition declarations after line 210:
src.models.config.UserDataCollection --* src.models.config.Configuration : user_data_collection
+src.models.config.QuotaLimiterConfiguration --* src.models.config.QuotaHandlersConfiguration : limiters
+src.models.config.QuotaSchedulerConfiguration --* src.models.config.QuotaHandlersConfiguration : scheduler
@endumlAlso applies to: 205-210
🤖 Prompt for AI Agents
In docs/config.puml around lines 205-210 (and ensure coverage for the
QuotaHandlersConfiguration declaration at ~138-144), add missing composition
relationships for the two composed fields: create composition edges from
src.models.config.QuotaHandlersConfiguration to
src.models.config.QuotaLimiterConfiguration for the "limiters" field, and from
src.models.config.QuotaHandlersConfiguration to
src.models.config.QuotaSchedulerConfiguration for the "scheduler" field; place
these declarations immediately after line 210 so the diagram shows that
QuotaHandlersConfiguration composes Limiters and Scheduler using the appropriate
fully-qualified class names and relationship labels.
| class "QuotaSchedulerConfiguration" as src.models.config.QuotaSchedulerConfiguration { | ||
| period : Annotated | ||
| } |
There was a problem hiding this comment.
Missing inheritance declaration for QuotaSchedulerConfiguration.
The QuotaSchedulerConfiguration class is defined but lacks an inheritance declaration. According to the codebase patterns and learnings, all configuration classes must extend ConfigurationBase.
Add the missing inheritance declaration after line 200:
src.models.config.QuotaLimiterConfiguration --|> src.models.config.ConfigurationBase
+src.models.config.QuotaSchedulerConfiguration --|> src.models.config.ConfigurationBase
src.models.config.SQLiteDatabaseConfiguration --|> src.models.config.ConfigurationBaseBased on learnings.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/config.puml around lines 152 to 154, the QuotaSchedulerConfiguration
class is missing its inheritance; update the class declaration to extend
ConfigurationBase (i.e., add "extends ConfigurationBase" to the class line) and
ensure any necessary stereotype/namespace references match the rest of the
diagram; place the inheritance declaration as instructed after line 200 if that
aligns with the file's ordering conventions.
1 participant
Description
LCORE-741: Proper quota limiters configuration
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Tests
Documentation