Repository navigation
Conversation
mwien
left a comment
mwien
left a comment
There was a problem hiding this comment.
Do you have tests with enable_reasoning = True especially in integration/e2e tests or sample code?
Apart from that just small nitpicks (see below).
| """ | ||
| Content criteria to evaluate. | ||
|
|
||
| Note: Granite Guardian requires separate calls for each configured content category. |
There was a problem hiding this comment.
[pp] Maybe "Granite Guardian performs separate calls for each configured content category." is clearer in case that is meant. "requires" sounds a bit like the user has to call it multiple times.
There was a problem hiding this comment.
It is resolved in the latest version.
|
|
||
| class GraniteGuardian41Categories(BaseModel): | ||
| """ | ||
| Content criteria to evaluate. |
There was a problem hiding this comment.
[pp] mention Granite Guardian in docstring.
There was a problem hiding this comment.
It is resolved in the latest version.
yamaceay
left a comment
yamaceay
left a comment
There was a problem hiding this comment.
everything is in place, just small comments
| from gen_ai_hub.orchestration_v2.models.base import ABCBaseModel as BaseModel | ||
|
|
||
|
|
||
| class GraniteGuardian41Categories(BaseModel): |
There was a problem hiding this comment.
[req] spec enforces minProperties: 1, I would like to know whether it would be a good practice to add Pydantic validation to this class: it should have at least one of them to be set to True. Maybe one test case with empty initialization would be nice too, it should fail if every category is False.
There was a problem hiding this comment.
The idea of current implementation is to follow the same schema as the LlamaGuard38bFilter, where minProperties: 1 also required. The server also accepts the all false situations.
There was a problem hiding this comment.
Hm, I get that it is a general pattern. Would still make sense to add a @model_validator function for these classes (if not use a common validator for these)? If minProperties: 1 is set in spec and server accepts zero, it's weird and unintended so far as I understood.
There was a problem hiding this comment.
ah, I see the points. I understand minProperties: 1 only requires the object to contain at least one required keys — it counts keys, not values, so an all-false config still satisfies it (the keys are present), whereas an empty {} is rejected.
Context
Closes SAP/ai-sdk-python-backlog#56.
What this PR does and why it is needed
This PR adds IBM Granite Guardian 4.1 as a new self-hosted content filter provider for the content filtering module, alongside the existing Azure Content Safety and Llama Guard 3 providers. Unit tests, integration tests are also included.
Definition of Done