test(guardrails): Add request logging integration tests#760
Open
JashG wants to merge 1 commit into
Open
Conversation
Signed-off-by: Jash Gulabrai <jgulabrai@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds integration tests for VirtualModel guardrail request logging, covering field selection, populated structures, merged rail data, response placement, omission, and invalid options. ChangesRequest logging integration coverage
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-guardrails/tests/integration/test_request_logging.py`:
- Around line 264-277: Update test_input_and_output_logs_merged_for_list_fields
to inspect log["internal_events"] by phase, asserting that both input and output
internal-event entries are present rather than only checking the total length.
Preserve the existing activated_rails phase assertions and other log-option
coverage.
- Around line 304-342: The test test_unknown_log_field_rejected should catch
nemo_platform.APIStatusError instead of a generic Exception, then assert the 422
error detail identifies options.log.unknown and states “Extra inputs are not
permitted.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e0477730-f774-4abe-9c9d-0376b4a70839
📒 Files selected for processing (1)
plugins/nemo-guardrails/tests/integration/test_request_logging.py
Comment on lines
+264
to
+277
| def test_input_and_output_logs_merged_for_list_fields(self, igw_plugin_harness: IGWPluginHarness) -> None: | ||
| """List-shaped log fields should concatenate input-rail and output-rail entries.""" | ||
| response, _ = self._invoke_all_safe_request( | ||
| igw_plugin_harness, | ||
| log_options={"activated_rails": True, "llm_calls": True, "internal_events": True}, | ||
| ) | ||
|
|
||
| log = self._log_dict(response) | ||
|
|
||
| activated_rail_types = {rail["type"] for rail in log["activated_rails"]} | ||
| assert "input" in activated_rail_types | ||
| assert "output" in activated_rail_types | ||
| assert len(log["llm_calls"]) >= 2 | ||
| assert len(log["internal_events"]) >= 2 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify internal_events came from both middleware phases.
len(...) >= 2 cannot prove merging: one rail may emit multiple internal events. Assert phase-specific input and output events, as already done for activated_rails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/nemo-guardrails/tests/integration/test_request_logging.py` around
lines 264 - 277, Update test_input_and_output_logs_merged_for_list_fields to
inspect log["internal_events"] by phase, asserting that both input and output
internal-event entries are present rather than only checking the total length.
Preserve the existing activated_rails phase assertions and other log-option
coverage.
Comment on lines
+304
to
+342
| def test_unknown_log_field_rejected(self, igw_plugin_harness: IGWPluginHarness) -> None: | ||
| """Unsupported log options should fail request validation.""" | ||
| harness = igw_plugin_harness | ||
| test_data_names = make_guardrails_test_data_names(workspace=harness.workspace) | ||
|
|
||
| harness.mock_chat_completions( | ||
| test_data_names.main_model_entity_ref, | ||
| responses=[ChatCompletion(body=chat_completion(content="No"))], | ||
| ) | ||
| harness.add_provider( | ||
| workspace=harness.workspace, | ||
| name=test_data_names.model_provider_name, | ||
| served_models={test_data_names.main_model_served_name: test_data_names.main_model_served_name}, | ||
| ) | ||
|
|
||
| guardrail_config = make_guardrail_config( | ||
| harness.workspace, | ||
| test_data_names.guardrail_config_name, | ||
| data=self._config_data(main_base_url=harness.nim_base_url), | ||
| ) | ||
|
|
||
| with harness.load_plugin(GUARDRAILS_PLUGIN_NAME): | ||
| harness.add_virtual_model( | ||
| workspace=harness.workspace, | ||
| name=test_data_names.request_virtual_model_name, | ||
| default_model_entity=test_data_names.main_model_entity_ref, | ||
| request_middleware=[make_middleware_call(guardrail_config)], | ||
| ) | ||
| with pytest.raises(Exception) as exc_info: | ||
| harness.chat_completions( | ||
| workspace=harness.workspace, | ||
| body={ | ||
| "model": test_data_names.request_virtual_model_name, | ||
| "messages": [{"role": "user", "content": self.USER_INPUT}], | ||
| "guardrails": {"options": {"log": {"unknown": True}}}, | ||
| }, | ||
| ) | ||
|
|
||
| assert "422" in str(exc_info.value) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'pytest\.raises\(' plugins/nemo-guardrails/tests/integration
rg -n -C4 'def chat_completions\(' --type=pyRepository: NVIDIA-NeMo/nemo-platform
Length of output: 4464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,350p' plugins/nemo-guardrails/tests/integration/test_request_logging.py
printf '\n--- middleware caching pattern ---\n'
sed -n '110,150p' plugins/nemo-guardrails/tests/integration/test_middleware_config_caching.py
printf '\n--- upstream error surfacing pattern ---\n'
sed -n '180,240p' plugins/nemo-guardrails/tests/integration/test_upstream_error_surfacing.py
printf '\n--- search validation exception uses ---\n'
rg -n 'APIStatusError|422|unknown' plugins/nemo-guardrails/tests/integration plugins/nemo-guardrails -g '*.py'Repository: NVIDIA-NeMo/nemo-platform
Length of output: 10809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' plugins/nemo-guardrails/src/nemo_guardrails_plugin/requests.py
printf '\n--- unit rails validation cases ---\n'
sed -n '90,140p' plugins/nemo-guardrails/tests/unit/test_rails.py
printf '\n--- middleware validation cases ---\n'
sed -n '620,690p' plugins/nemo-guardrails/tests/unit/test_middleware.pyRepository: NVIDIA-NeMo/nemo-platform
Length of output: 8217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the harness implementation and relevant exception handling.
rg -n -C3 'def chat_completions\(' plugins/nemo-guardrails -g '*.py'
rg -n -C3 'InferenceMiddlewareError|APIStatusError|BadRequestError' plugins/nemo-guardrails -g '*.py'
# Inspect the harness method if present in repo files.
fd -a '.*harness.*\.py|.*plugin.*harness.*\.py' plugins/nemo-guardrailsRepository: NVIDIA-NeMo/nemo-platform
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'class .*Harness|chat_completions\(' plugins/nemo-guardrails -g '*.py'
rg -n -C4 'APIStatusError' plugins/nemo-guardrails/tests -g '*.py'Repository: NVIDIA-NeMo/nemo-platform
Length of output: 50381
Assert the specific 422 detail plugins/nemo-guardrails/tests/integration/test_request_logging.py:332-342 should catch nemo_platform.APIStatusError and match options.log.unknown / Extra inputs are not permitted, not just any 422.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/nemo-guardrails/tests/integration/test_request_logging.py` around
lines 304 - 342, The test test_unknown_log_field_rejected should catch
nemo_platform.APIStatusError instead of a generic Exception, then assert the 422
error detail identifies options.log.unknown and states “Extra inputs are not
permitted.”
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds integration tests to validate the Guardrails plugin request-logging behavior
Summary by CodeRabbit