feat: Add responses and safety impl extra_body #3781
Conversation
meta-cla
bot
added
the
CLA Signed
slekkala1
requested review from
ashwinb,
bbrowning,
ehhuang,
franciscojavierarceo,
hardikjshah,
leseb,
mattf,
raghotham,
reluctantfuturist,
terrytangyuan and
yanxi0830
as code owners
| # Shields parameter received via extra_body - not yet implemented | ||
| if shields is not None: | ||
| raise NotImplementedError("Shields parameter is not yet implemented in the meta-reference provider") | ||
| shield_ids = extract_shield_ids(shields) if shields else [] |
There was a problem hiding this comment.
given that we would like to reuse the moderations API -- could we reconsider naming this parameter perhaps? I think people use guardrails much more (even in OpenAI's agents-sdk). I wonder if we should use that?
slekkala1
force-pushed
the
new-responses-and-safety
branch
2 times, most recently
from
76f0478 to
76b991c
Compare
| :param type: The type/identifier of the guardrail. | ||
| """ | ||
|
|
||
| type: str |
There was a problem hiding this comment.
just for my learning: what types are available?
There was a problem hiding this comment.
not sure about this part @ashwinb, I usually only know identifier for a shield, as such only supporting that for now. May be this is to allow more fields in future.
| include: list[str] | None = None, | ||
| max_infer_iters: int | None = 10, | ||
| shields: list | None = None, | ||
| guardrails: list | None = None, |
There was a problem hiding this comment.
could you type the list more exactly?
| if isinstance(guardrail, str): | ||
| guardrail_ids.append(guardrail) | ||
| elif isinstance(guardrail, ResponseGuardrailSpec): | ||
| guardrail_ids.append(guardrail.type) |
There was a problem hiding this comment.
this seems confusing: type being used as id. Is there a better way to name this?
llama_stack/providers/inline/agents/meta_reference/responses/utils.py
Outdated
Show resolved
Hide resolved
|
|
||
| violation_message = await run_multiple_guardrails(self.safety_api, text, self.guardrail_ids) | ||
| if violation_message: | ||
| logger.info(f"{context.capitalize()} guardrail violation: {violation_message}") |
There was a problem hiding this comment.
nit: just add this log to run_multiple_guardrails and we don't need this extra wrapper _apply_guardrails function
There was a problem hiding this comment.
Just when I think I fixed all the redundancies by claude, something still slips...
|
|
||
| # Input safety validation - check messages before processing | ||
| if self.guardrail_ids: | ||
| combined_text = interleaved_content_as_str([msg.content for msg in self.ctx.messages]) |
There was a problem hiding this comment.
should we document somewhere that guardrails only apply to text input?
There was a problem hiding this comment.
yes the shield + moderation apis dont support the image, this is known tech debt, I filed an issue for that before.
llama_stack/providers/inline/agents/meta_reference/responses/streaming.py
Outdated
Show resolved
Hide resolved
| ) + tool_call.function.arguments | ||
|
|
||
| # Output Safety Validation for a chunk | ||
| if chat_response_content: |
There was a problem hiding this comment.
we should just check for self.guardrail_ids first as it makes it clear what this block is for and prevents unnecessary work
| accumulated_text = "".join(chat_response_content) | ||
| violation_message = await self._apply_guardrails(accumulated_text, "output") | ||
| if violation_message: | ||
| yield await self._create_refusal_response(violation_message) |
There was a problem hiding this comment.
Are the output chunks already yielded by this point?
There was a problem hiding this comment.
ResponseTextDeltaEvent are streamed, the output chunk is not yet streamed by this point. I initially had this check within the delta in the above loop, but apparently that is too expensive, so moved to here for a chunk
There was a problem hiding this comment.
Updated as discussed to not emit delta events when guardrails are configured
slekkala1
force-pushed
the
new-responses-and-safety
branch
2 times, most recently
from
e72c6e4 to
d462e96
Compare
slekkala1
force-pushed
the
new-responses-and-safety
branch
from
d462e96 to
a9ebdfe
Compare
3 participants
What does this PR do?
Have closed the previous PR due to merge conflicts with multiple PRs
Addressed all comments from #3768 (sorry for carrying over to this one)
Test Plan
Added UTs and integration tests