Skip to content

Conversation

Elbehery
Copy link
Contributor

@Elbehery Elbehery commented Aug 25, 2025

What does this PR do?

This PR removes llama_guard from mypy exclude list and fix all type errors to enable strict type checking for the LlamaGuard safety provider implementation.

Changes:

  • Add missing shield_store attribute to satisfy Safety protocol
  • Fix function parameter and return type annotations
  • Add proper type handling for mixed content types (text/image)
  • Fix OpenAI message conversion with correct async handling
  • Add proper error handling and validation for API responses
  • Add mypy to dev dependencies for consistent type checking

Part of #2647

Supersedes #2676

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 25, 2025
@Elbehery Elbehery changed the title chore: enable mypy type checking for llama_guard safety provider chore: enable mypy type checking for llama_guard safety provider Aug 25, 2025
@Elbehery Elbehery force-pushed the 20250825_add_mypy_llama_guard branch 6 times, most recently from 8455e1d to 27c2eb2 Compare August 27, 2025 20:37
@Elbehery Elbehery force-pushed the 20250825_add_mypy_llama_guard branch from 27c2eb2 to 0543f74 Compare August 29, 2025 00:18
@slekkala1
Copy link
Contributor

@Elbehery any tooling used to generate these changes? can you declare ?

@Elbehery
Copy link
Contributor Author

@Elbehery any tooling used to generate these changes? can you declare ?

hello ✋🏽

i remove the file from pyproject.toml, then i run mypy on the file to see the errors

then i use Claude code to help resolving

@@ -271,7 +275,7 @@ def get_safety_categories(self) -> list[str]:

return final_categories

def validate_messages(self, messages: list[Message]) -> None:
def validate_messages(self, messages: list[Message]) -> list[Message]:
Copy link
Contributor

@slekkala1 slekkala1 Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this was named validate_messages in the first place, seems this function is altering the messages and returning messages, can we rename this to process_messages?

@@ -136,6 +136,8 @@


class LlamaGuardSafetyImpl(Safety, ShieldsProtocolPrivate):
shield_store: ShieldStore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shield_store exists in the parent class of Safety already, do we need this here as well?

elif isinstance(c, ImageContentItem):
if most_recent_img is None and m.role == Role.user.value:
most_recent_img = c
content.append(c)
# Note: we handle images separately for vision models
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove the comment, this function is only for text input?

| OpenAIToolMessageParam
| OpenAIDeveloperMessageParam
)
openai_messages = [cast(openai_message_param, openai_message)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move the conversion logic to a different function? Also, move all the imports to top of the module?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants