chore(modelarmor): refactored test cases for sanitization #13314
Conversation
product-auto-label
bot
added
the
samples
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @mihirvala-crestdata, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request focuses on refactoring the test cases for sanitization in the Model Armor snippets. The changes include adding RAI filter test cases for both user prompt and model response sanitization, removing irrelevant assertions, and updating relevant assertions for PI&J filter and advanced SDP sanitization test cases. Additionally, basic and advanced SDP user prompt sanitization test cases have been added. The template fixture simple_template was replaced with all_filter_template in several tests.
Highlights
- RAI Filter Test Cases: Added new test cases to cover RAI (Responsible AI) filters for user prompts and model responses, ensuring proper sanitization.
- SDP Sanitization Test Cases: Implemented basic and advanced SDP (Sensitive Data Protection) user prompt sanitization test cases to validate the effectiveness of SDP filters.
- Assertion Updates: Refined assertions in existing test cases, particularly for PI&J (Personally Identifiable & Jailbreak) filters and advanced SDP, to improve test accuracy.
- Template Fixture Replacement: Replaced the
simple_templatefixture withall_filter_templatein multiple tests to ensure comprehensive filter coverage.
Changelog
- model_armor/snippets/list_templates_with_filter.py
- Corrected a typo in the argument description from 'Model Armour Template ID(s)' to 'Model Armor Template ID(s)'.
- model_armor/snippets/snippets_test.py
- Added RAI filter test case for user prompt sanitization.
- Added RAI filter test case for model response sanitization.
- Removed irrelevant assertions from test cases.
- Added/updated relevant assertions for PI&J filter and advance SDP sanitization test cases.
- Added basic and advance SDP user prompt sanitization test case.
- Replaced
simple_templatefixture withall_filter_templateintest_get_template,test_list_templates,test_update_templates,test_delete_template,test_update_model_armor_template_metadata,test_update_model_armor_template_labels,test_update_model_armor_template_with_mask_configuration,test_sanitize_user_prompt_with_malicious_url_template,test_sanitize_user_prompt_with_csam_template, andtest_sanitize_user_prompt_with_jail_break_template. - Corrected
sdr_inspect_template_idtosdp_inspect_template_idintest_create_model_armor_template_with_advanced_sdp.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In realms of code, where filters reside,
Tests ensure that nothing can hide.
RAI and SDP,
A shield for you and me,
Sanitizing data, far and wide.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request refactors test cases for sanitization in the model armor snippets. It includes adding RAI filter test cases, updating assertions, and adding basic and advanced SDP user prompt sanitization test cases. Overall, the changes seem well-structured and improve the test coverage for the sanitization functionalities. However, there are a few areas that could be improved for clarity and efficiency.
Summary of Findings
- Inconsistent Naming: The naming of variables and test functions is inconsistent, which can lead to confusion. For example,
sdr_inspect_template_idandsdp_inspect_template_idare used interchangeably. Standardizing these names would improve readability. - Redundant Assertions: Some assertions appear to be redundant or not directly related to the test's purpose. Removing these would simplify the tests and make them easier to understand.
- Magic strings: Magic strings are used in assertions, which reduces readability and maintainability. Consider defining these strings as constants to improve code clarity.
Merge Readiness
The pull request is almost ready for merging. However, addressing the inconsistencies in naming, removing redundant assertions, and replacing magic strings with constants would significantly improve the code quality and maintainability. I am unable to approve this pull request, and recommend that another reviewer also approves this code before merging. At a minimum, the high severity issues should be addressed before merging.
mihirvala-crestdata
force-pushed
the
model-armor-tests-refactor
branch
from
c616850 to
7bba0ac
Compare
| """ | ||
| template_id, _ = basic_sdp_template | ||
|
|
||
| user_prompt = "Give me email associated with following ITIN: 988-86-1234 " |
There was a problem hiding this comment.
| user_prompt = "Give me email associated with following ITIN: 988-86-1234 " | |
| user_prompt = "Give me email associated with following ITIN: 988-86-1234" |
| ) | ||
|
|
||
|
|
||
| def test_sanitize_user_prompt_with_basic_sdp_template( |
There was a problem hiding this comment.
In this test, we can additionally match info type as well. Refer to testSanitizeUserPromptWithBasicSdpTemplate test in Java.
| == modelarmor_v1.FilterMatchState.MATCH_FOUND | ||
| ) | ||
|
|
||
| assert ( |
There was a problem hiding this comment.
Can we assert the readacted output here instead?
Ref: https://github.com/GoogleCloudPlatform/java-docs-samples/blob/159d49a7b51d0e20f0a1e19c009d98e986a40d0f/modelarmor/src/test/java/modelarmor/SnippetsIT.java#L499
| @@ -762,18 +855,39 @@ def test_sanitize_user_prompt_with_empty_template( | |||
| ) -> None: | |||
| template_id, _ = empty_template | |||
|
|
|||
| user_prompt = ( | |||
| "Can you describe this link? https://testsafebrowsing.appspot.com/s/malware.html," | |||
| user_prompt = "Can you describe this link? https://testsafebrowsing.appspot.com/s/malware.html," | |||
There was a problem hiding this comment.
| user_prompt = "Can you describe this link? https://testsafebrowsing.appspot.com/s/malware.html," | |
| user_prompt = "Can you describe this link? https://testsafebrowsing.appspot.com/s/malware.html" |
| model_response = ( | ||
| "You can use this to make a cake: https://testsafebrowsing.appspot.com/s/malware.html," | ||
| ) | ||
| model_response = "You can use this to make a cake: https://testsafebrowsing.appspot.com/s/malware.html," |
There was a problem hiding this comment.
| model_response = "You can use this to make a cake: https://testsafebrowsing.appspot.com/s/malware.html," | |
| model_response = "You can use this to make a cake: https://testsafebrowsing.appspot.com/s/malware.html" |
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request refactors test cases for sanitization, adding new tests for RAI filters and updating assertions for existing tests. Overall, the changes improve the test coverage and clarity. However, there are some inconsistencies and areas for improvement, particularly in naming conventions, redundant assertions, and missing assertions.
Summary of Findings
- Inconsistent Naming Convention: The naming convention for template IDs is inconsistent. Some use 'Model Armour' while others use 'Model Armor'. Consistent naming improves readability and maintainability.
- Redundant Assertions: Several tests include assertions that check the template name against a predefined format. These assertions are redundant and can be removed to simplify the tests.
- Missing Assertions: Some tests lack specific assertions to validate the behavior of the functions under test. Adding relevant assertions ensures that the tests effectively verify the functionality.
- Incorrect Filter Results: The test_sanitize_user_prompt_with_empty_template test asserts that the csam filter result should be NO_MATCH_FOUND, but this is incorrect since the csam filter is not being used in this test.
- Duplicated code: The code for setting up floor settings is duplicated across project, organization, and folder levels. This duplication increases the risk of inconsistencies and makes maintenance more difficult.
Merge Readiness
The pull request is not yet ready for merging. The inconsistencies in naming conventions and the redundant assertions should be addressed. Additionally, missing assertions should be added to ensure that the tests effectively verify the functionality. I am unable to approve this pull request, and users should have others review and approve this code before merging.
| response.sanitization_result.filter_match_state | ||
| == modelarmor_v1.FilterMatchState.NO_MATCH_FOUND | ||
| ) | ||
| assert ( |
There was a problem hiding this comment.
Add assertion of match state for all categories of RAI as well, here as well as in model response test.
Had received this comment from Utsav on Java snippet: GoogleCloudPlatform/java-docs-samples#10065 (comment)
Description
Changelog:
Checklist
nox -s py-3.9(see Test Environment Setup)nox -s lint(see Test Environment Setup)