feat: first pass at guardian io contract subclasses#1357
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
There was a problem hiding this comment.
if this is deprecated, do we need to update or should we just leave alone?
There was a problem hiding this comment.
Yes, even though we're testing deprecation since we updated the signature on in guardian.py we will fail 6/8 tests with TypeErrors unless we update the def fake_call_intrinsic function.
ajbozarth
left a comment
There was a problem hiding this comment.
Some feedback from Claude — follow-ups inline.
One non-blocking flag on the PR description (transient, so just checking intent): the "100% coverage" checkbox and the issue's "19 tests unchanged" criterion don't match the final state — guardian.py is at 98.51%, and test_guardian_documents.py is 17 tests after the XOR cases moved into the contract tests. Did you mean to leave those as-is?
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
ajbozarth
left a comment
There was a problem hiding this comment.
LGTM, but I'd defer final approval to others who've spent more time in the guardian code
|
I'll give Jake a day to add his 2C, otherwise can merge and iterate later if need be. |
psschwei
left a comment
There was a problem hiding this comment.
I assume this is true:
matching the output schema documented in the HF README for each adapter
but would be confirm in the PR body
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class _PolicyGuardrailsContract(IOContract): |
There was a problem hiding this comment.
General comment / observation : in this PR we're implementing one contract class per intrinsic, whereas in #1321 they were more generic (for dict and list return types). My gut instinct is that it's probably better to be a little more generic if possible, but I'll also tag in @jakelorocco for his opinion.
There was a problem hiding this comment.
Yeah; I'm not quite certain how I feel about the super specific ones. I'm fine starting out this way but we may need to relax things in the future. Ultimately, we do have to specify this somewhere.
Pull Request
Issue
Fixes #1332
Description
Adds io contract subclasses, adapter constants, and utilizes these via existing call sites in guardian.py
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.