feat: added topic scoping to the SDK #466
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdded four optional topic-scoping parameters to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🤖 Fix all issues with AI agents
In `@qualifire/client.py`:
- Around line 66-69: Document and/or constrain topic_scoping_target and add an
explicit enable flag: update the signature and docs around topic_scoping_target
to either (a) accept free-form topic names and explicitly state that in the
docstring for topic_scoping_target/allowed_topics, or (b) replace Optional[str]
with a new enum (e.g., TopicScopingTarget) that lists permitted values (similar
to PolicyTarget) and update all references accordingly; also add a boolean
topic_scoping_check parameter (default False) to mirror other checks and make
enabling explicit (treat allowed_topics as ignored unless topic_scoping_check is
True), and update validation logic where topic_scoping_target, allowed_topics,
and topic_scoping_multi_turn_mode are used to enforce these rules.
🧹 Nitpick comments (1)
qualifire/client.py (1)
112-115: Consider clarifying the docstring fortopic_scoping_target.The docstring for
topic_scoping_targetsays "Target topic for topic scoping check" which doesn't clarify what values are accepted. Consider documenting:
- Valid values (e.g., "input", "output", "both" or free-form text)
- How the parameters work together (e.g., is
allowed_topicsrequired whentopic_scoping_modeis set?)
| topic_scoping_mode: Optional[ModelMode] = None, | ||
| topic_scoping_multi_turn_mode: bool = False, | ||
| topic_scoping_target: Optional[str] = None, | ||
| allowed_topics: Optional[List[str]] = None, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if TopicScopingTarget enum exists
ast-grep --pattern 'class TopicScopingTarget'Repository: qualifire-dev/qualifire-python-sdk
Length of output: 60
🏁 Script executed:
# Check PolicyTarget enum definition and usage
rg -n 'PolicyTarget' --type=py -A 3 -B 1Repository: qualifire-dev/qualifire-python-sdk
Length of output: 1232
🏁 Script executed:
# Check the evaluate() method signature to see all parameters
rg -n 'def evaluate' --type=py -A 20Repository: qualifire-dev/qualifire-python-sdk
Length of output: 1771
🏁 Script executed:
# Check what other checks have boolean flags
rg -n '_check: bool' --type=pyRepository: qualifire-dev/qualifire-python-sdk
Length of output: 1836
🏁 Script executed:
# Look for EvaluationRequest class to understand parameter mapping
rg -n 'class EvaluationRequest' --type=py -A 30Repository: qualifire-dev/qualifire-python-sdk
Length of output: 2366
🏁 Script executed:
# Check docstrings in client.py for topic scoping parameters
rg -n 'topic_scoping' --type=py -B 2 -A 2Repository: qualifire-dev/qualifire-python-sdk
Length of output: 2215
🏁 Script executed:
# Check if there are any comments or validation related to topic_scoping_target values
rg -n 'topic.scoping.*target' --type=py -i -B 3 -A 3Repository: qualifire-dev/qualifire-python-sdk
Length of output: 2118
🏁 Script executed:
# Check the complete evaluate method signature including docstring
sed -n '38,120p' qualifire/client.pyRepository: qualifire-dev/qualifire-python-sdk
Length of output: 5370
Document valid values for topic_scoping_target and consider adding an explicit enable flag for consistency.
The topic_scoping_target parameter is typed as Optional[str] without constraints or documentation of valid values. In contrast, policy_target (line 65) uses a PolicyTarget enum to clarify accepted values (input/output/both). If topic_scoping_target accepts free-form topic names, this should be documented explicitly. Alternatively, if values are constrained, define an enum for better SDK usability.
Additionally, all other checks in the API have explicit boolean enable flags (e.g., grounding_check, pii_check, content_moderation_check), but topic scoping lacks a topic_scoping_check: bool parameter. This creates ambiguity around enabling topic scoping—is it implicitly enabled when allowed_topics is provided, or should an explicit flag be required for consistency?
🤖 Prompt for AI Agents
In `@qualifire/client.py` around lines 66 - 69, Document and/or constrain
topic_scoping_target and add an explicit enable flag: update the signature and
docs around topic_scoping_target to either (a) accept free-form topic names and
explicitly state that in the docstring for topic_scoping_target/allowed_topics,
or (b) replace Optional[str] with a new enum (e.g., TopicScopingTarget) that
lists permitted values (similar to PolicyTarget) and update all references
accordingly; also add a boolean topic_scoping_check parameter (default False) to
mirror other checks and make enabling explicit (treat allowed_topics as ignored
unless topic_scoping_check is True), and update validation logic where
topic_scoping_target, allowed_topics, and topic_scoping_multi_turn_mode are used
to enforce these rules.
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
LGTM, left one minor comment
qualifire/client.py
Outdated
| policy_target: PolicyTarget = PolicyTarget.BOTH, | ||
| topic_scoping_mode: Optional[ModelMode] = None, | ||
| topic_scoping_multi_turn_mode: bool = False, | ||
| topic_scoping_target: Optional[str] = None, |
There was a problem hiding this comment.
Can you use an enum / Literal for the target? like the PolicyTarget
There was a problem hiding this comment.
Don't forget to bump the version in pyproject.toml
2 participants
Description
Related Issue
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.make codestyle.Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.