-
Notifications
You must be signed in to change notification settings - Fork 68
feat(rlsapi): add method to combine request inputs #898
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
WalkthroughAdded RlsapiV1InferRequest.get_input_source(), which builds a single string by concatenating non-empty sources (question, context.stdin, context.attachments.contents, context.terminal.output) in that priority order, separated by double newlines. Added unit tests covering combinations and edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4)src/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/models/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/{unit,integration}/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)tests/unit/models/rlsapi/test_requests.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 (2)
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 |
|
Hi @major. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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: 0
🧹 Nitpick comments (2)
src/models/rlsapi/requests.py (2)
3-10: Logger initialized but never used.The module logger is defined following the correct pattern, but it's not used anywhere in the file. The new
get_input_source()method doesn't emit any log statements.Consider either:
- Removing the logger if not needed yet
- Adding a comment explaining it's reserved for future use
- Adding debug logging in
get_input_source()if useful for troubleshooting
182-204: Implementation looks good with minor style suggestion.The method correctly concatenates non-empty input sources in the documented priority order using double newlines. The logic is clear and the docstring is comprehensive.
Minor suggestion: The
# pylint: disable=no-membercomment on line 197 could be moved to line 182 (at the function level) for better scope clarity:def get_input_source(self) -> str: """Combine all non-empty input sources into a single string. Joins question, stdin, attachment contents, and terminal output with double newlines, in priority order. Empty sources are omitted. Priority order: 1. question 2. stdin 3. attachment contents 4. terminal output Returns: The combined input string with sources separated by double newlines. """ + # pylint: disable=no-member parts = [ self.question, self.context.stdin, self.context.attachments.contents, self.context.terminal.output, ] return "\n\n".join(part for part in parts if part)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/models/rlsapi/requests.py(2 hunks)tests/unit/models/rlsapi/test_requests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/models/rlsapi/test_requests.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/models/rlsapi/test_requests.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/models/rlsapi/requests.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/rlsapi/requests.py
🧬 Code graph analysis (2)
tests/unit/models/rlsapi/test_requests.py (1)
src/models/rlsapi/requests.py (5)
RlsapiV1InferRequest(127-204)RlsapiV1Context(93-124)RlsapiV1Attachment(12-29)RlsapiV1Terminal(32-43)get_input_source(182-204)
src/models/rlsapi/requests.py (1)
src/models/config.py (2)
config(324-341)ConfigurationBase(33-36)
⏰ 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)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (1)
tests/unit/models/rlsapi/test_requests.py (1)
316-463: Excellent test coverage for the new method.The test suite comprehensively validates
get_input_source():
- Parameterized tests cover all meaningful input combinations (all four sources through question-only)
- Edge cases verify multiline content preservation and strict priority ordering
- Factory fixture pattern keeps tests clean and readable
The tests follow pytest best practices and align well with the coding guidelines.
Joins non-empty sources (question, stdin, attachment, terminal) with double newlines for LLM inference. Signed-off-by: Major Hayden <major@redhat.com>
tisnik
left a 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.
LGTM
|
/ok-to-test |
Description
Adds input handling for the rlsapi v1 interface. Joins non-empty sources (question, stdin, attachment, terminal) with double newlines for LLM inference.
Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
Existing unit tests should be sufficient.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.