feat: audio synthesis, transforms, and NVVoiceChat target for audio probing - #9
feat: audio synthesis, transforms, and NVVoiceChat target for audio probing#9codingshreyash wants to merge 4 commits into
Conversation
9b4ad8c to
f09f8b6
Compare
f09f8b6 to
5d1eb3e
Compare
…oiceChat target Infrastructure for the audio probe family (no probes yet): - resources/audio: pluggable TTS synthesis, signal transforms, attack provenance/recipes, audio validation, spoken-reliability case data - generators: audio-format interface (base.py supported_formats, openai.py), OpenAI-compatible NVVoiceChat audio target (nim.py) Based on feature/technique_intent. First of a 3-PR stack (foundation -> probes+detectors -> injection). Assisted-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Shreyash Ranjan <shrranjan@nvidia.com>
5d1eb3e to
8176bcb
Compare
jmartin-tech
left a comment
There was a problem hiding this comment.
Some initial thoughts while I work on getting a test target up and running.
Still working thru the resources/audio utility here to understand what is being provided. Will likely need a pass to understand the contracts of the methods and another to evaluate the code inside them.
I also suspect much of the _call_model() code here can be reworked to utilized the parent method and do pre and post processing as most of it looks to be the shared.
| pipeline, but any returned audio is ignored. | ||
| """ | ||
|
|
||
| ENV_VAR = "NIM_API_KEY" |
| "request_timeout": 120, # S2S endpoints can be slow | ||
| "request_retries": 2, |
There was a problem hiding this comment.
The openai.client request parameter would pass a timeout configured with each request, I see this is registered on the client creation but is not really needed.
_call_model() should implement backoff expectations the request_retries here diverges from garak generator common behavior and is probably not needed.
| def _load_unsafe(self): | ||
| # voice shims may not implement /models, so don't enumerate on empty name | ||
| self.client = openai.OpenAI( | ||
| base_url=self.uri, | ||
| api_key=self.api_key, | ||
| timeout=getattr(self, "request_timeout", 120), | ||
| max_retries=getattr(self, "request_retries", 2), | ||
| ) | ||
| if self.name in ("", None): | ||
| raise ValueError( | ||
| f"{self.generator_family_name} requires model name to be set, " | ||
| "e.g. --target_name <model-served-by-the-shim>" | ||
| ) | ||
| self.generator = self.client.chat.completions |
There was a problem hiding this comment.
This again should not be needed as noted timeout should just be exposed as a default param and request_retries should be managed by _call_model() backoff configuration.
| "spelled correctly and does the shim accept input_audio?" | ||
| ) | ||
| logging.critical(msg, exc_info=e) | ||
| raise GarakException(f"\U0001f6d1 {msg}") from e |
There was a problem hiding this comment.
Do not place control characters in exception messages.
| if tool_calls: | ||
| text = self._serialise_tool_calls(tool_calls, content) | ||
| else: | ||
| text = content if isinstance(content, str) else "" |
There was a problem hiding this comment.
This is not a sustainable way to represent response content or tool calls. Serializing the response into a string and stuffing into a text field is not viable to release as an accepted pattern.
This would tightly couple the any paired detector to data this specific generator implements without any formal structure while also placing the data in a location other detectors would attempt to process it.
| # this test drives a text prompt; skip generators that | ||
| # require non-text input (e.g. audio-only S2S targets) | ||
| text_conv = Conversation( | ||
| [Turn("user", Message("first testing string"))] | ||
| ) | ||
| try: | ||
| class_instance._prepare_prompt(text_conv) | ||
| except Exception: | ||
| continue |
There was a problem hiding this comment.
This should test attributes of the class not execution to filter incompatible generators.
| def test_openai_compatible_reports_supported_audio_formats(): | ||
| assert OpenAICompatible.supported_formats("audio") == { | ||
| "wav", | ||
| "mp3", | ||
| }, "reports audio formats through the generator format interface" | ||
| assert ( | ||
| OpenAICompatible.supported_formats("image") == set() | ||
| ), "reports no image formats by default" | ||
|
|
||
|
|
||
| def test_openai_audio_compatible_declares_audio_modality(): | ||
| assert OpenAICompatible.modality["in"] == { | ||
| "text" | ||
| }, "generic compatible targets remain text-only at the harness boundary" | ||
| assert OpenAIAudioCompatible.modality["in"] == { | ||
| "text", | ||
| "audio", | ||
| }, "explicit audio-compatible targets accept text plus audio" | ||
| assert OpenAIAudioCompatible.supported_formats("audio") == { | ||
| "wav", | ||
| "mp3", | ||
| }, "audio-compatible targets inherit supported wire formats" |
There was a problem hiding this comment.
These test checks static metadata of the generator. Test should validate the contract of the unit not the static implementation details.
Also hardcoded values should not be asserted additions to the class list should not require expansion of the test unless it was due to some explicit restriction.
| @staticmethod | ||
| def _append_wav_silence(wav_bytes: bytes, silence_ms: int) -> bytes: | ||
| """Return wav_bytes with silence_ms milliseconds of silence appended.""" | ||
| with wave.open(io.BytesIO(wav_bytes)) as wf: | ||
| params = wf.getparams() | ||
| original_frames = wf.readframes(wf.getnframes()) | ||
|
|
||
| silence_frames = int(params.framerate * silence_ms / 1000) | ||
| silence_bytes = b"\x00" * silence_frames * params.nchannels * params.sampwidth | ||
|
|
||
| buf = io.BytesIO() | ||
| with wave.open(buf, "wb") as wf_out: | ||
| wf_out.setparams(params) | ||
| wf_out.writeframes(original_frames + silence_bytes) | ||
| return buf.getvalue() |
There was a problem hiding this comment.
This seems like something that belongs in the resource utility.
| "n", | ||
| "frequency_penalty", | ||
| "presence_penalty", | ||
| "timeout", |
There was a problem hiding this comment.
Seems odd to both inject timeout at the client level from a different mapped param and suppress it here.
| @staticmethod | ||
| def _serialise_tool_calls(tool_calls, content) -> str: | ||
| serialised = [] | ||
| for tc in tool_calls: | ||
| if hasattr(tc, "model_dump"): | ||
| serialised.append(tc.model_dump()) | ||
| elif isinstance(tc, dict): | ||
| serialised.append(tc) | ||
| else: | ||
| serialised.append(str(tc)) | ||
| payload = {"tool_calls": serialised} | ||
| if isinstance(content, str) and content: | ||
| payload["content"] = content | ||
| return json.dumps(payload, sort_keys=True, default=str) |
There was a problem hiding this comment.
This is not a sustainable way to represent response content. Serializing the response into an string and stuffing into a text field is not viable to release as an accepted pattern.
Reuse the shared OpenAI request path, preserve tool calls as structured message metadata, and move WAV silence padding into audio resources. Update the affected tests to exercise behavior contracts and class metadata. Assisted-by: OpenAI Codex Signed-off-by: Shreyash Ranjan <shrranjan@nvidia.com>
jmartin-tech
left a comment
There was a problem hiding this comment.
Think of many of these comments as design questions.
The revisions here that attempt to introduce first calls tool calls to Message objects are likely somewhat controversial. There is an open PR in the upstream repo exploring how Attempt and Message should add this support, it also links to an example of how another fork has been exploring this.
Similar to the last review recommendation are focused on existing garak patterns, the audio resources library code still needs a review pass targeted at what the code enables.
| if tool_call is not None: | ||
| normalised.append(tool_call) | ||
| self.notes["tool_calls"] = normalised | ||
|
|
There was a problem hiding this comment.
Why is this here? simply adding a tool_calls member to the data class would be sufficient. We should not build some sort of backwards compatible layer for new functionality that is introduced.
tool_calls: Optional[list[ToolCall]] = field(default_factory=list)| def test_supported_formats_reports_declared_modality_contract(): | ||
| formats = _FormatAwareGenerator.supported_formats("audio") | ||
|
|
||
| assert formats == {"test-audio"}, "reports formats declared for the modality" | ||
| formats.add("caller-only") | ||
| assert _FormatAwareGenerator.audio_formats == { | ||
| "test-audio" | ||
| }, "returns a copy so callers cannot mutate generator metadata" |
There was a problem hiding this comment.
Not sure what this is testing or why it is a expected to be a contract of the Generator class.
Python interpreter features are not unit test validation canidates.
| enumerate_models_on_missing_name = True | ||
| warn_on_unqualified_name = True |
There was a problem hiding this comment.
This is not a feature this generator needs, behavior changes the generator not in scope for the goal of the PR should not be injected.
Also note even if in scope these should be DEFAULT_PARAMS entries not class attributes.
| msg = "Model call didn't match endpoint expectations, see log" | ||
| logging.critical(msg, exc_info=uee) | ||
| raise GarakException(f"🛑 {msg}") from uee | ||
| raise GarakException(msg) from uee |
There was a problem hiding this comment.
Do not change error messages not related to the PR goal.
| msg = "NIM generation failed. Is the model name spelled correctly?" | ||
| logging.critical(msg, exc_info=oe) | ||
| raise GarakException(f"🛑 {msg}") from oe | ||
| raise GarakException(msg) from nfe |
There was a problem hiding this comment.
Do not change error messages not related to the PR goal.
| if audio_msg.data_path is not None: | ||
| audio_path = Path(audio_msg.data_path) | ||
| if not audio_path.is_file(): | ||
| raise GarakException( | ||
| f"{self.__class__.__name__} audio file not found: {audio_path}" | ||
| ) | ||
| fmt = audio_path.suffix.lower().lstrip(".") | ||
| if fmt not in self.audio_formats: | ||
| raise GarakException( | ||
| f"{self.__class__.__name__} expected one of " | ||
| f"{sorted(self.audio_formats)} audio formats: {audio_path}" | ||
| ) | ||
| raw = audio_path.read_bytes() | ||
| else: |
There was a problem hiding this comment.
Determining a file type by extension is not portable if you have a file then it should be inspected for type, there is already built in support for this used in the else branch below related to the data_type tuple.
I suspect this code may have been intended as a runtime memory saving action, however this file extension check is too brittle and should avoided. The branch below is all that is needed.
| ) | ||
| raw = audio_path.read_bytes() | ||
| else: | ||
| raw = audio_msg.data |
There was a problem hiding this comment.
This does not need to hold the raw data in variable, only ensure it has been accessed once to be sure data_type is populated.
audio_msg.data # access the data member to ensure it was loaded by the Message
In theory this should not be required and the Message object could be enhanced to always ensure if data_path or data was used to set the binary content that data_type will return a populated value.
| configured_extra_body = getattr(self, "extra_body", None) | ||
| extra_body = ( | ||
| dict(configured_extra_body) | ||
| if isinstance(configured_extra_body, dict) | ||
| else {} | ||
| ) | ||
| if self.generate_audio: | ||
| extra_body.setdefault("generate_audio", True) | ||
| self.extra_body = extra_body |
There was a problem hiding this comment.
Why is this here? manipulating self.extra_body every time a prompt is processed is not really a desired side-effect this method should be introducing.
| ) -> list[ToolCall]: | ||
| """Return provider-neutral tool calls from an OpenAI-style response.""" | ||
|
|
||
| del content |
There was a problem hiding this comment.
Why accept a parameter to immediately mark for garbage collection?
I suspect this is due to how the nim generator serializes the tool calls in a response. The base class does not need that implementation detail and the extending class should be structured to handle that without introducing responsibilities to the base class.
| @classmethod | ||
| def from_value(cls, value: object) -> Optional["ToolCall"]: | ||
| """Normalise a structured call object or dictionary.""" | ||
|
|
||
| if isinstance(value, cls): | ||
| return value if value.name.strip() else None | ||
| if not isinstance(value, dict): | ||
| return None | ||
|
|
||
| function = value.get("function") | ||
| payload = function if isinstance(function, dict) else value | ||
| name = payload.get("name") | ||
| if not isinstance(name, str) or not name.strip(): | ||
| return None |
There was a problem hiding this comment.
That is this really meant to do? This looks related to the preemptive backwards compatible notes storage for tool calls that was noted is likely unnecessary.
Assisted-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Shreyash Ranjan <shrranjan@nvidia.com>
|
Test failures for dd4a870 are a dependency introduced issue with python 3.10. |
Avoid the LiteLLM 1.97.0 Pydantic regression on Python 3.10 while leaving newer Python versions unconstrained. Assisted-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Shreyash Ranjan <shrranjan@nvidia.com>
What this changes
Adds shared audio infrastructure for the follow-up probe PRs. This PR does not add probes.
In
resources/audio/:In
generators/:NVVoiceChat, a NIM speech-to-speech target that sends WAV data in aninput_audioblock and reads the returned text transcriptNVVoiceChatis audio-in/text-out. It requires--target_nameand does not select a default target.First-class tool-call storage on
AttemptorMessageis deliberately out of scope. That data-model work is being explored in NVIDIA#2058; this PR does not introduce a competing representation.Why
garak does not currently have a reusable audio-input path for generated adversarial candidates. This PR adds the shared path for synthesizing and transforming audio, validating candidate quality, sending it to an audio-capable target, and passing returned text to existing detectors.
The audio probes in #10 and #11 build on this infrastructure.
Duplicate-work check
This does not duplicate an open upstream PR. Searches for
audio synthesis NVVoiceChat,audio input, andspeech inputfound no open NVIDIA/garak PR implementing this foundation. NVIDIA#2058 covers the separateAttempt/Messagetool-call model and is intentionally not duplicated here.Tests
python -m pytest -q tests/test_attempt.py tests/generators/test_generators_base.py tests/generators/test_openai_compatible.py tests/generators/test_nim_voicechat.py tests/resources/test_audio_attack.py tests/resources/test_audio_synthesis.py tests/resources/test_audio_transforms.py tests/resources/test_audio_validation.py— 83 passedpython -m pytest -q tests/test_docs.py— 682 passedpython -m pytest -q tests/generators/test_generators.py— 120 passed, 2 skippedpython -m black --check <changed Python files>— passedgit diff --check— passedNotes for review
This branch is based on
feature/technique_intentbecause follow-up probes useIntentProbe, which is not onmainyet.This is the first of three related PRs:
#10 and #11 depend on this PR. PR #9 can land independently.
AI assistance
OpenAI Codex assisted with review analysis, implementation, and test execution. The human submitter is responsible for reviewing every changed line, understanding the design, and validating the commands above before upstream submission.