feat(checks): add ContainsAny and ContainsAll builtin checks (#2361)#2380
Open
abhigyan631 wants to merge 2 commits intoGiskard-AI:mainfrom
Open
feat(checks): add ContainsAny and ContainsAll builtin checks (#2361)#2380abhigyan631 wants to merge 2 commits intoGiskard-AI:mainfrom
abhigyan631 wants to merge 2 commits intoGiskard-AI:mainfrom
Conversation
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces two new check implementations, ContainsAny and ContainsAll, which verify if specific keywords exist within a text string. These checks support Unicode normalization and case-insensitive matching. Feedback suggests refactoring the classes to a common base to reduce code duplication, pre-formatting values for better performance, and handling potential serialization issues with the NoMatch object in the result details. Additionally, it was recommended to ensure that missing values in ContainsAll are reported uniquely for better clarity.
Comment on lines
+22
to
+194
| @Check.register("contains_any") | ||
| class ContainsAny[InputType, OutputType, TraceType: Trace]( # pyright: ignore[reportMissingTypeArgument] | ||
| Check[InputType, OutputType, TraceType] | ||
| ): | ||
| """Check that passes if the text contains AT LEAST ONE value from the list. | ||
|
|
||
| Examples | ||
| -------- | ||
| Direct text and values:: | ||
|
|
||
| check = ContainsAny( | ||
| text="Machine learning is a subset of AI.", | ||
| values=["machine learning", "ML", "artificial intelligence"], | ||
| case_sensitive=False, | ||
| ) | ||
|
|
||
| Extract text from trace:: | ||
|
|
||
| check = ContainsAny( | ||
| text_key="trace.last.outputs.response", | ||
| values=["consult a doctor", "medical advice"], | ||
| ) | ||
| """ | ||
|
|
||
| text: str | None = Field( | ||
| default=None, | ||
| description="The text string to search within. If None, extracted from trace using text_key.", | ||
| ) | ||
| text_key: str = Field( | ||
| default="trace.last.outputs", | ||
| description="JSONPath expression to extract the text from the trace.", | ||
| ) | ||
| values: list[str] = Field( | ||
| description="List of strings to check against. Passes if at least one is found.", | ||
| ) | ||
| normalization_form: NormalizationForm | None = Field( | ||
| default="NFKC", | ||
| description="Unicode normalization form to apply (NFC, NFD, NFKC, NFKD). Defaults to NFKC.", | ||
| ) | ||
| case_sensitive: bool = Field( | ||
| default=False, | ||
| description="If True, matching is case-sensitive. Defaults to False.", | ||
| ) | ||
|
|
||
| def _format_str(self, value: str) -> str: | ||
| """Format a string for matching by applying normalization and case handling.""" | ||
| value = normalize_string(value, self.normalization_form) | ||
| if not self.case_sensitive: | ||
| value = value.lower() | ||
| return value | ||
|
|
||
| @override | ||
| async def run(self, trace: TraceType) -> CheckResult: | ||
| """Execute the contains-any check.""" | ||
| text = provided_or_resolve( | ||
| trace, key=self.text_key, value=provide_not_none(self.text) | ||
| ) | ||
|
|
||
| details = {"text": text, "values": self.values} | ||
|
|
||
| if isinstance(text, NoMatch): | ||
| return CheckResult.failure( | ||
| message=f"No value found for text key '{self.text_key}'.", | ||
| details=details, | ||
| ) | ||
|
|
||
| if not isinstance(text, str): | ||
| return CheckResult.failure( | ||
| message=f"Value for text key '{self.text_key}' is not a string, got {type(text)}.", | ||
| details=details, | ||
| ) | ||
|
|
||
| formatted_text = self._format_str(text) | ||
|
|
||
| for value in self.values: | ||
| if self._format_str(value) in formatted_text: | ||
| return CheckResult.success( | ||
| message=f"The text contains '{value}'.", | ||
| details={**details, "matched": value}, | ||
| ) | ||
|
|
||
| return CheckResult.failure( | ||
| message=f"The text does not contain any of: {self.values}.", | ||
| details=details, | ||
| ) | ||
|
|
||
|
|
||
| @Check.register("contains_all") | ||
| class ContainsAll[InputType, OutputType, TraceType: Trace]( # pyright: ignore[reportMissingTypeArgument] | ||
| Check[InputType, OutputType, TraceType] | ||
| ): | ||
| """Check that passes if the text contains EVERY value from the list. | ||
|
|
||
| Examples | ||
| -------- | ||
| Direct text and values:: | ||
|
|
||
| check = ContainsAll( | ||
| text="The dosage is 200mg. Consult a doctor before use.", | ||
| values=["dosage", "mg", "consult"], | ||
| case_sensitive=False, | ||
| ) | ||
|
|
||
| Extract text from trace:: | ||
|
|
||
| check = ContainsAll( | ||
| text_key="trace.last.outputs.response", | ||
| values=["definition", "example"], | ||
| ) | ||
| """ | ||
|
|
||
| text: str | None = Field( | ||
| default=None, | ||
| description="The text string to search within. If None, extracted from trace using text_key.", | ||
| ) | ||
| text_key: str = Field( | ||
| default="trace.last.outputs", | ||
| description="JSONPath expression to extract the text from the trace.", | ||
| ) | ||
| values: list[str] = Field( | ||
| description="List of strings to check against. Passes only if every item is found.", | ||
| ) | ||
| normalization_form: NormalizationForm | None = Field( | ||
| default="NFKC", | ||
| description="Unicode normalization form to apply (NFC, NFD, NFKC, NFKD). Defaults to NFKC.", | ||
| ) | ||
| case_sensitive: bool = Field( | ||
| default=False, | ||
| description="If True, matching is case-sensitive. Defaults to False.", | ||
| ) | ||
|
|
||
| def _format_str(self, value: str) -> str: | ||
| """Format a string for matching by applying normalization and case handling.""" | ||
| value = normalize_string(value, self.normalization_form) | ||
| if not self.case_sensitive: | ||
| value = value.lower() | ||
| return value | ||
|
|
||
| @override | ||
| async def run(self, trace: TraceType) -> CheckResult: | ||
| """Execute the contains-all check.""" | ||
| text = provided_or_resolve( | ||
| trace, key=self.text_key, value=provide_not_none(self.text) | ||
| ) | ||
|
|
||
| details = {"text": text, "values": self.values} | ||
|
|
||
| if isinstance(text, NoMatch): | ||
| return CheckResult.failure( | ||
| message=f"No value found for text key '{self.text_key}'.", | ||
| details=details, | ||
| ) | ||
|
|
||
| if not isinstance(text, str): | ||
| return CheckResult.failure( | ||
| message=f"Value for text key '{self.text_key}' is not a string, got {type(text)}.", | ||
| details=details, | ||
| ) | ||
|
|
||
| formatted_text = self._format_str(text) | ||
|
|
||
| missing = [v for v in self.values if self._format_str(v) not in formatted_text] | ||
|
|
||
| if missing: | ||
| return CheckResult.failure( | ||
| message=f"The text is missing: {missing}.", | ||
| details={**details, "missing": missing}, | ||
| ) | ||
|
|
||
| return CheckResult.success( | ||
| message=f"The text contains all of: {self.values}.", | ||
| details=details, | ||
| ) |
Contributor
There was a problem hiding this comment.
There is significant code duplication between ContainsAny and ContainsAll. Both classes share identical fields (text, text_key, values, normalization_form, case_sensitive), the _format_str method, and the initial logic in the run method.
Consider refactoring this by introducing a private base class (e.g., _BaseContainsCheck) to encapsulate the common fields and helper methods. This would significantly improve maintainability and reduce the risk of inconsistent updates in the future.
Comment on lines
+80
to
+101
| details = {"text": text, "values": self.values} | ||
|
|
||
| if isinstance(text, NoMatch): | ||
| return CheckResult.failure( | ||
| message=f"No value found for text key '{self.text_key}'.", | ||
| details=details, | ||
| ) | ||
|
|
||
| if not isinstance(text, str): | ||
| return CheckResult.failure( | ||
| message=f"Value for text key '{self.text_key}' is not a string, got {type(text)}.", | ||
| details=details, | ||
| ) | ||
|
|
||
| formatted_text = self._format_str(text) | ||
|
|
||
| for value in self.values: | ||
| if self._format_str(value) in formatted_text: | ||
| return CheckResult.success( | ||
| message=f"The text contains '{value}'.", | ||
| details={**details, "matched": value}, | ||
| ) |
Contributor
There was a problem hiding this comment.
The run method has a few areas for improvement:
- Efficiency:
self._format_str(value)is called inside the loop. Sinceself.valuesand the formatting settings are constant for the check instance, these should be pre-formatted once to avoid redundant regex and normalization calls during execution. - Serialization: The
detailsdictionary includes thetextvariable even if it is aNoMatchobject. This can lead to serialization errors (e.g., when converting to JSON) ifNoMatchis not a serializable type. It is safer to useNoneor a string representation for theNoMatchcase.
Comment on lines
+167
to
+189
| details = {"text": text, "values": self.values} | ||
|
|
||
| if isinstance(text, NoMatch): | ||
| return CheckResult.failure( | ||
| message=f"No value found for text key '{self.text_key}'.", | ||
| details=details, | ||
| ) | ||
|
|
||
| if not isinstance(text, str): | ||
| return CheckResult.failure( | ||
| message=f"Value for text key '{self.text_key}' is not a string, got {type(text)}.", | ||
| details=details, | ||
| ) | ||
|
|
||
| formatted_text = self._format_str(text) | ||
|
|
||
| missing = [v for v in self.values if self._format_str(v) not in formatted_text] | ||
|
|
||
| if missing: | ||
| return CheckResult.failure( | ||
| message=f"The text is missing: {missing}.", | ||
| details={**details, "missing": missing}, | ||
| ) |
Contributor
There was a problem hiding this comment.
The run method can be optimized and improved for clarity:
- Efficiency: Pre-format
self.valuesto avoid redundant calls to_format_strin the list comprehension. - Serialization: Handle
NoMatchindetailsto prevent potential serialization errors. - Clarity: If
self.valuescontains duplicates that are missing from the text, they are currently reported multiple times in themissinglist. Using unique values for themissingreport is generally clearer for the user.
abhigyan631
force-pushed
the
feat/contains-any-all-checks
branch
from
68df0cc to
c061197
Compare
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
1 participant
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.
Description
Summary
Added two new built-in string checks (
ContainsAnyandContainsAll) to validate output text against a list of strings. This enables evaluating topic coverage and disclaimers natively without writing custom lambda functions.Implementation Details
contains_matching.pyfollowing the standardStringMatchingpattern.NFKCdefault), and case-sensitivity settings.from giskard.checks import ContainsAny, ContainsAllTesting
test_contains_matching.pysuite.Related Issue
Resolves #2361
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.uv.lockrunninguv lock(only applicable whenpyproject.tomlhas beenmodified)