tests: add path/validate and v3 checks#60
Conversation
Reviewer's GuideThis PR adds end-to-end validation and testing scripts to ensure that previous code review fixes are correctly implemented and remain robust. It introduces a V3 test harness to programmatically verify regex replacements, checkpoint handling, id2label sorting, import cleanup, path expansion, API timeouts, PII prevention, and syntax across key scripts; a validator script checking that hardcoded paths, interactive login, and error handling comments have been addressed (with bonus improvements); and a dedicated path detection test covering various environment scenarios and tilde expansion. Sequence diagram for code review validation executionsequenceDiagram
participant User
participant ValidatorScript as validate_code_review_fixes.py
participant UploadScript as upload_model_to_huggingface.py
User->>ValidatorScript: Run validation script
ValidatorScript->>UploadScript: Open and read code
ValidatorScript->>ValidatorScript: Run portability, login, error handling checks
ValidatorScript->>User: Output validation summary and exit code
Class diagram for the new validation script structureclassDiagram
class validate_code_review_fixes {
+validate_comment_1_portability()
+validate_comment_2_interactive_login()
+validate_comment_3_error_handling()
+validate_additional_improvements()
+main()
}
validate_code_review_fixes : -script_path
validate_code_review_fixes : -validators
validate_code_review_fixes : -results
Class diagram for test harness and path detection scriptsclassDiagram
class test_code_review_fixes_v3 {
+test_regex_replacements()
+test_checkpoint_handling()
+test_id2label_sorting()
+test_import_cleanup()
+test_path_expansion()
+test_api_timeouts()
+test_pii_prevention()
+test_syntax()
}
class test_model_path_detection {
+test_environment_scenarios()
+test_tilde_expansion()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThree new Python scripts were introduced under the deployment scripts directory. These scripts provide automated tests and validations for code review fixes, model path detection logic, and code review comment compliance. Each script contains multiple functions targeting specific aspects such as environment variable handling, error management, interactive login checks, model checkpoint compatibility, and prevention of sensitive information exposure. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as test_code_review_fixes_v3.py
participant Codebase as Deployment Scripts
Tester->>Codebase: Run regex replacement tests
Tester->>Codebase: Validate config file creation
Tester->>Codebase: Test DataParallel checkpoint handling
Tester->>Codebase: Check id2label mapping handling
Tester->>Codebase: Verify unused import cleanup
Tester->>Codebase: Test path expansion logic
Tester->>Codebase: Simulate API timeout handling
Tester->>Codebase: Scan for PII exposure
Tester->>Codebase: Validate syntax of modified files
Codebase-->>Tester: Pass/fail results per test
Tester->>Tester: Aggregate and report summary
sequenceDiagram
participant Validator as validate_code_review_fixes.py
participant UploadScript as upload_model_to_huggingface.py
Validator->>UploadScript: Check for environment variable usage
Validator->>UploadScript: Validate interactive login handling
Validator->>UploadScript: Inspect error handling logic
Validator->>UploadScript: Detect additional improvements
UploadScript-->>Validator: Code snippets and logic
Validator->>Validator: Aggregate validation results
Validator->>Console: Print summary and exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @uelkerd, 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!
I've added new test and validation scripts to ensure that recent code review fixes (V3) are correctly implemented and that model path detection works as expected. This significantly enhances the robustness, reliability, and maintainability of our deployment scripts by providing automated checks for critical functionalities and past issues.
Highlights
- Comprehensive V3 Fix Validation: A new test suite (test_code_review_fixes_v3.py) has been introduced to validate critical fixes from a recent code review. This includes checks for robust regex-based model replacement, correct handling of DataParallel checkpoints, proper id2label key processing, cleanup of unused imports, accurate path expansion, API server timeout mechanisms, and prevention of PII exposure in error responses.
- Model Path Detection Testing: A dedicated test script (test_model_path_detection.py) has been added to rigorously test the get_model_base_directory function. It verifies that model base directories are correctly identified under various conditions, including auto-detection, environment variable settings (SAMO_DL_BASE_DIR, MODEL_BASE_DIR), and tilde (~) expansion for home directories.
- Code Review Fix Validation Script: A new validation script (validate_code_review_fixes.py) has been implemented to programmatically verify that specific code review comments related to portability, interactive login behavior, and state dictionary error handling have been addressed in the upload_model_to_huggingface.py script. It also checks for additional improvements like multiple token environment variables and better error messages.
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 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 or fill out our survey 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.
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. ↩
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test scripts to validate code review fixes and improvements. The focus is on ensuring that previously identified code review comments have been properly addressed through automated validation.
- Adds validation scripts for code review fixes with specific checks for portability, interactive login handling, and error handling
- Implements path detection testing with environment variable configuration validation
- Provides comprehensive v3 validation covering regex improvements, DataParallel support, and security enhancements
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
scripts/deployment/validate_code_review_fixes.py |
Main validation script that checks if three specific code review comments have been addressed |
scripts/deployment/test_model_path_detection.py |
Tests portable model directory detection logic with various environment variable scenarios |
scripts/deployment/test_code_review_fixes_v3.py |
Comprehensive validation suite for advanced fixes including regex patterns, DataParallel handling, and security improvements |
Comments suppressed due to low confidence (1)
scripts/deployment/test_code_review_fixes_v3.py:84
- This path construction logic is fragile and will fail if temp_base is not under the user's home directory. The replace operation may not work as expected, potentially creating invalid paths.
"deployment_type": "huggingface_hub",
| print("\n🏠 Test 4: With home directory path expansion") | ||
|
|
||
| # Create a temporary directory under the expanded home path to ensure it exists | ||
| with tempfile.TemporaryDirectory() as temp_base: |
There was a problem hiding this comment.
Missing import for tempfile module. The code uses tempfile.TemporaryDirectory() but only imports TemporaryDirectory from tempfile at line 39. Add import tempfile to the imports section.
| if any(error_indicator in context.lower() for error_indicator in ['error', 'exception', 'failed', 'timeout']): | ||
| # Check if this is actually a successful response (should contain emotion) | ||
| if '"emotion"' not in context: | ||
| pii_exposures.append(f"Line {i+1}: {line.strip()}") |
There was a problem hiding this comment.
The nested loop checking for PII exposures could be inefficient for large files. Consider using a more efficient approach like regex search or breaking early when error indicators are found.
| pii_exposures.append(f"Line {i+1}: {line.strip()}") | |
| # Use regex to find all occurrences of '"text": text' | |
| for match in re.finditer(r'"text":\s*text', content): | |
| start_idx = max(0, match.start() - 500) | |
| end_idx = min(len(content), match.end() + 500) | |
| context = content[start_idx:end_idx] | |
| # Check for error indicators in context | |
| if re.search(r'error|exception|failed|timeout', context, re.IGNORECASE): | |
| # Check if this is actually a successful response (should contain emotion) | |
| if not re.search(r'"emotion"', context): | |
| # Find line number for reporting | |
| line_num = content[:match.start()].count('\n') + 1 | |
| pii_exposures.append(f"Line {line_num}: {match.group().strip()}") |
| # Check for hardcoded paths (should be minimal/none) | ||
| hardcoded_indicators = [ | ||
| content.count('/Users/') <= 1, # Allow one or fewer hardcoded /Users/ paths | ||
| content.count('/home/') <= 1, # Allow one or fewer hardcoded /home/ paths |
There was a problem hiding this comment.
[nitpick] The hardcoded path detection logic uses magic numbers (1) without clear justification. Consider making these thresholds configurable or adding comments explaining why exactly one occurrence is acceptable.
| content.count('/home/') <= 1, # Allow one or fewer hardcoded /home/ paths | |
| # Allow up to MAX_ALLOWED_HARDCODED_PATHS occurrences for legacy or placeholder paths. | |
| hardcoded_indicators = [ | |
| content.count('/Users/') <= MAX_ALLOWED_HARDCODED_PATHS, | |
| content.count('/home/') <= MAX_ALLOWED_HARDCODED_PATHS, |
| improvements = [] | ||
|
|
||
| # Check that duplicate sys import is removed | ||
| sys_import_count = content.count("import sys") |
There was a problem hiding this comment.
Using string counting to detect imports is unreliable as it could match imports in comments or strings. Consider using AST parsing to accurately detect actual import statements.
| sys_import_count = content.count("import sys") | |
| sys_import_count = 0 | |
| try: | |
| tree = ast.parse(content) | |
| for node in ast.walk(tree): | |
| if isinstance(node, ast.Import): | |
| for alias in node.names: | |
| if alias.name == "sys": | |
| sys_import_count += 1 | |
| except Exception as e: | |
| print(f" ❌ AST parsing error: {e}") | |
| return False |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @uelkerd - I've reviewed your changes - here's some feedback:
- Refactor these print-and-return style scripts into a standard pytest/unittest suite with assert statements for clearer test results and CI integration.
- Extract repeated setup and teardown logic (e.g., TemporaryDirectory creation and env var cleanup) into shared helper functions or fixtures to reduce duplication.
- Use parametrized tests for similar scenarios (such as the id2label and regex replacement cases) to simplify loops and make each case more explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Refactor these print-and-return style scripts into a standard pytest/unittest suite with assert statements for clearer test results and CI integration.
- Extract repeated setup and teardown logic (e.g., TemporaryDirectory creation and env var cleanup) into shared helper functions or fixtures to reduce duplication.
- Use parametrized tests for similar scenarios (such as the id2label and regex replacement cases) to simplify loops and make each case more explicit.
## Individual Comments
### Comment 1
<location> `scripts/deployment/validate_code_review_fixes.py:28` </location>
<code_context>
+ ]
+
+ # Check for hardcoded paths (should be minimal/none)
+ hardcoded_indicators = [
+ content.count('/Users/') <= 1, # Allow one or fewer hardcoded /Users/ paths
+ content.count('/home/') <= 1, # Allow one or fewer hardcoded /home/ paths
+ 'configurable' in content.lower(),
+ 'environment variable' in content.lower()
+ ]
+
</code_context>
<issue_to_address>
Allowing one hardcoded path may mask issues in larger scripts.
Flag all hardcoded path occurrences, or refine the check to differentiate between test and production code.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Check for hardcoded paths (should be minimal/none)
hardcoded_indicators = [
content.count('/Users/') <= 1, # Allow one or fewer hardcoded /Users/ paths
content.count('/home/') <= 1, # Allow one or fewer hardcoded /home/ paths
'configurable' in content.lower(),
'environment variable' in content.lower()
]
=======
# Check for hardcoded paths (should be none in production code)
is_test_code = (
'test' in script_path.lower() or
'unittest' in content.lower() or
'pytest' in content.lower()
)
hardcoded_users_count = content.count('/Users/')
hardcoded_home_count = content.count('/home/')
hardcoded_paths_flagged = False
if not is_test_code:
# Flag if any hardcoded path is present in production code
hardcoded_paths_flagged = (hardcoded_users_count > 0 or hardcoded_home_count > 0)
else:
# In test code, allow hardcoded paths
hardcoded_paths_flagged = False
hardcoded_indicators = [
not hardcoded_paths_flagged,
'configurable' in content.lower(),
'environment variable' in content.lower()
]
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `scripts/deployment/validate_code_review_fixes.py:75` </location>
<code_context>
+ content = f.read()
+
+ # Check for non-interactive environment detection
+ interactive_checks = [
+ 'is_interactive_environment' in content,
+ 'CI' in content and 'DOCKER' in content, # Environment checks
+ 'KUBERNETES' in content,
+ 'sys.stdin.isatty()' in content,
+ 'non-interactive' in content.lower()
+ ]
+
</code_context>
<issue_to_address>
Environment detection logic may be too simplistic.
Instead of searching for environment variable names as substrings, consider detecting explicit environment variable checks or conditional statements for more reliable environment detection.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Check for non-interactive environment detection
interactive_checks = [
'is_interactive_environment' in content,
'CI' in content and 'DOCKER' in content, # Environment checks
'KUBERNETES' in content,
'sys.stdin.isatty()' in content,
'non-interactive' in content.lower()
]
=======
# Check for explicit non-interactive environment detection logic
import re
env_var_patterns = [
r'os\.environ\.get\(["\']CI["\']\)',
r'os\.environ\.get\(["\']DOCKER["\']\)',
r'os\.environ\.get\(["\']KUBERNETES["\']\)',
r'os\.getenv\(["\']CI["\']\)',
r'os\.getenv\(["\']DOCKER["\']\)',
r'os\.getenv\(["\']KUBERNETES["\']\)',
r'if\s+["\']CI["\']\s+in\s+os\.environ',
r'if\s+["\']DOCKER["\']\s+in\s+os\.environ',
r'if\s+["\']KUBERNETES["\']\s+in\s+os\.environ',
r'sys\.stdin\.isatty\(\)'
]
interactive_checks = [
any(re.search(pattern, content) for pattern in env_var_patterns),
'is_interactive_environment' in content,
'non-interactive' in content.lower()
]
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `scripts/deployment/validate_code_review_fixes.py:93` </location>
<code_context>
+ ]
+
+ # Check for user consent before interactive login
+ user_consent_checks = [
+ 'input(' in content, # User input for consent
+ 'Attempt interactive login' in content,
+ 'y/N' in content or 'yes/no' in content
+ ]
+
</code_context>
<issue_to_address>
User consent detection may yield false positives.
The current approach may match unrelated code. Please refine the detection to target explicit consent prompts or logic directly associated with login attempts.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Check for user consent before interactive login
user_consent_checks = [
'input(' in content, # User input for consent
'Attempt interactive login' in content,
'y/N' in content or 'yes/no' in content
]
=======
# Check for explicit user consent prompts or logic directly associated with login attempts
user_consent_checks = [
# Look for input prompts that explicitly ask for consent/agreement/permission
any(
kw in content
for kw in [
"input('Do you consent",
'input("Do you consent',
"input('Do you agree",
'input("Do you agree',
"input('Allow interactive login",
'input("Allow interactive login',
"input('Grant permission",
'input("Grant permission',
]
),
# Look for interactive login attempts that mention consent in their prompt
"interactive login" in content and (
"consent" in content or "agree" in content or "permission" in content
),
]
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `scripts/deployment/validate_code_review_fixes.py:136` </location>
<code_context>
+ content = f.read()
+
+ # Check for error handling around state dict loading
+ error_handling_patterns = [
+ 'try:' in content and 'except' in content,
+ 'RuntimeError' in content,
+ 'size mismatch' in content,
+ 'KeyError' in content,
+ 'Architecture mismatch' in content
+ ]
+
</code_context>
<issue_to_address>
Error handling detection may miss context-specific exceptions.
Consider identifying exception blocks that directly handle state dict loading to improve detection accuracy.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Check for error handling around state dict loading
error_handling_patterns = [
'try:' in content and 'except' in content,
'RuntimeError' in content,
'size mismatch' in content,
'KeyError' in content,
'Architecture mismatch' in content
]
=======
# Improved detection: Check for exception blocks directly handling state dict loading
import re
# Find try-except blocks that contain state dict loading
state_dict_load_patterns = [
r'try\s*:\s*\n(?:[^\n]*\n)*?\s*(torch\.load|model\.load_state_dict)\s*\(',
]
exception_patterns = [
r'except\s+(RuntimeError|KeyError|ValueError|Exception)',
]
found_error_handling = False
for try_pattern in state_dict_load_patterns:
for match in re.finditer(try_pattern, content, re.MULTILINE):
# Find the corresponding except block after the try block
try_block_end = match.end()
except_match = re.search(exception_patterns[0], content[try_block_end:])
if except_match:
found_error_handling = True
break
if found_error_handling:
break
error_handling_patterns = [found_error_handling]
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `scripts/deployment/validate_code_review_fixes.py:259` </location>
<code_context>
+
+ print(f"\nValidations passed: {passed}/{total}")
+
+ if passed >= 3: # Allow for additional improvements to be optional
+ print("\n🎉 ALL REQUIRED CODE REVIEW COMMENTS SUCCESSFULLY ADDRESSED!")
+ print("\n📋 Summary of fixes implemented:")
</code_context>
<issue_to_address>
Threshold for passing validations may be too lenient.
Permitting a single required validation to fail may allow critical issues to slip through. Recommend requiring all mandatory validations to pass, or explicitly separating required from optional checks.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if passed >= 3: # Allow for additional improvements to be optional
print("\n🎉 ALL REQUIRED CODE REVIEW COMMENTS SUCCESSFULLY ADDRESSED!")
print("\n📋 Summary of fixes implemented:")
print(" ✅ Comment 1: Hardcoded absolute paths → Environment variable configuration")
print(" ✅ Comment 2: Interactive login issues → Non-interactive environment detection")
print(" ✅ Comment 3: No state dict error handling → Comprehensive error handling")
print(" ✅ Bonus: Enhanced authentication, PyTorch compatibility, better error messages")
return True
print(f"\n⚠️ Only {passed}/{total} validations passed - some fixes may need review")
return False
=======
if passed == total:
print("\n🎉 ALL REQUIRED CODE REVIEW COMMENTS SUCCESSFULLY ADDRESSED!")
print("\n📋 Summary of fixes implemented:")
print(" ✅ Comment 1: Hardcoded absolute paths → Environment variable configuration")
print(" ✅ Comment 2: Interactive login issues → Non-interactive environment detection")
print(" ✅ Comment 3: No state dict error handling → Comprehensive error handling")
print(" ✅ Bonus: Enhanced authentication, PyTorch compatibility, better error messages")
return True
print(f"\n⚠️ Only {passed}/{total} validations passed - some fixes may need review")
return False
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `scripts/deployment/test_code_review_fixes_v3.py:24` </location>
<code_context>
+def test_regex_based_model_replacement():
</code_context>
<issue_to_address>
Consider adding negative test cases for regex replacement.
Adding tests for inputs that should not be replaced will help verify the regex does not match unintended patterns.
</issue_to_address>
### Comment 7
<location> `scripts/deployment/test_code_review_fixes_v3.py:71` </location>
<code_context>
+def test_config_file_creation():
</code_context>
<issue_to_address>
Test does not cover error conditions for config file creation.
Please add tests for scenarios like permission errors, invalid JSON, and missing directories to improve coverage of error handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Check for hardcoded paths (should be minimal/none) | ||
| hardcoded_indicators = [ | ||
| content.count('/Users/') <= 1, # Allow one or fewer hardcoded /Users/ paths | ||
| content.count('/home/') <= 1, # Allow one or fewer hardcoded /home/ paths | ||
| 'configurable' in content.lower(), | ||
| 'environment variable' in content.lower() | ||
| ] |
There was a problem hiding this comment.
suggestion: Allowing one hardcoded path may mask issues in larger scripts.
Flag all hardcoded path occurrences, or refine the check to differentiate between test and production code.
| # Check for hardcoded paths (should be minimal/none) | |
| hardcoded_indicators = [ | |
| content.count('/Users/') <= 1, # Allow one or fewer hardcoded /Users/ paths | |
| content.count('/home/') <= 1, # Allow one or fewer hardcoded /home/ paths | |
| 'configurable' in content.lower(), | |
| 'environment variable' in content.lower() | |
| ] | |
| # Check for hardcoded paths (should be none in production code) | |
| is_test_code = ( | |
| 'test' in script_path.lower() or | |
| 'unittest' in content.lower() or | |
| 'pytest' in content.lower() | |
| ) | |
| hardcoded_users_count = content.count('/Users/') | |
| hardcoded_home_count = content.count('/home/') | |
| hardcoded_paths_flagged = False | |
| if not is_test_code: | |
| # Flag if any hardcoded path is present in production code | |
| hardcoded_paths_flagged = (hardcoded_users_count > 0 or hardcoded_home_count > 0) | |
| else: | |
| # In test code, allow hardcoded paths | |
| hardcoded_paths_flagged = False | |
| hardcoded_indicators = [ | |
| not hardcoded_paths_flagged, | |
| 'configurable' in content.lower(), | |
| 'environment variable' in content.lower() | |
| ] |
| # Check for non-interactive environment detection | ||
| interactive_checks = [ | ||
| 'is_interactive_environment' in content, | ||
| 'CI' in content and 'DOCKER' in content, # Environment checks | ||
| 'KUBERNETES' in content, | ||
| 'sys.stdin.isatty()' in content, | ||
| 'non-interactive' in content.lower() | ||
| ] |
There was a problem hiding this comment.
suggestion: Environment detection logic may be too simplistic.
Instead of searching for environment variable names as substrings, consider detecting explicit environment variable checks or conditional statements for more reliable environment detection.
| # Check for non-interactive environment detection | |
| interactive_checks = [ | |
| 'is_interactive_environment' in content, | |
| 'CI' in content and 'DOCKER' in content, # Environment checks | |
| 'KUBERNETES' in content, | |
| 'sys.stdin.isatty()' in content, | |
| 'non-interactive' in content.lower() | |
| ] | |
| # Check for explicit non-interactive environment detection logic | |
| import re | |
| env_var_patterns = [ | |
| r'os\.environ\.get\(["\']CI["\']\)', | |
| r'os\.environ\.get\(["\']DOCKER["\']\)', | |
| r'os\.environ\.get\(["\']KUBERNETES["\']\)', | |
| r'os\.getenv\(["\']CI["\']\)', | |
| r'os\.getenv\(["\']DOCKER["\']\)', | |
| r'os\.getenv\(["\']KUBERNETES["\']\)', | |
| r'if\s+["\']CI["\']\s+in\s+os\.environ', | |
| r'if\s+["\']DOCKER["\']\s+in\s+os\.environ', | |
| r'if\s+["\']KUBERNETES["\']\s+in\s+os\.environ', | |
| r'sys\.stdin\.isatty\(\)' | |
| ] | |
| interactive_checks = [ | |
| any(re.search(pattern, content) for pattern in env_var_patterns), | |
| 'is_interactive_environment' in content, | |
| 'non-interactive' in content.lower() | |
| ] |
| # Check for user consent before interactive login | ||
| user_consent_checks = [ | ||
| 'input(' in content, # User input for consent | ||
| 'Attempt interactive login' in content, | ||
| 'y/N' in content or 'yes/no' in content | ||
| ] |
There was a problem hiding this comment.
suggestion: User consent detection may yield false positives.
The current approach may match unrelated code. Please refine the detection to target explicit consent prompts or logic directly associated with login attempts.
| # Check for user consent before interactive login | |
| user_consent_checks = [ | |
| 'input(' in content, # User input for consent | |
| 'Attempt interactive login' in content, | |
| 'y/N' in content or 'yes/no' in content | |
| ] | |
| # Check for explicit user consent prompts or logic directly associated with login attempts | |
| user_consent_checks = [ | |
| # Look for input prompts that explicitly ask for consent/agreement/permission | |
| any( | |
| kw in content | |
| for kw in [ | |
| "input('Do you consent", | |
| 'input("Do you consent', | |
| "input('Do you agree", | |
| 'input("Do you agree', | |
| "input('Allow interactive login", | |
| 'input("Allow interactive login', | |
| "input('Grant permission", | |
| 'input("Grant permission', | |
| ] | |
| ), | |
| # Look for interactive login attempts that mention consent in their prompt | |
| "interactive login" in content and ( | |
| "consent" in content or "agree" in content or "permission" in content | |
| ), | |
| ] |
| # Check for error handling around state dict loading | ||
| error_handling_patterns = [ | ||
| 'try:' in content and 'except' in content, | ||
| 'RuntimeError' in content, | ||
| 'size mismatch' in content, | ||
| 'KeyError' in content, | ||
| 'Architecture mismatch' in content | ||
| ] |
There was a problem hiding this comment.
suggestion: Error handling detection may miss context-specific exceptions.
Consider identifying exception blocks that directly handle state dict loading to improve detection accuracy.
| # Check for error handling around state dict loading | |
| error_handling_patterns = [ | |
| 'try:' in content and 'except' in content, | |
| 'RuntimeError' in content, | |
| 'size mismatch' in content, | |
| 'KeyError' in content, | |
| 'Architecture mismatch' in content | |
| ] | |
| # Improved detection: Check for exception blocks directly handling state dict loading | |
| import re | |
| # Find try-except blocks that contain state dict loading | |
| state_dict_load_patterns = [ | |
| r'try\s*:\s*\n(?:[^\n]*\n)*?\s*(torch\.load|model\.load_state_dict)\s*\(', | |
| ] | |
| exception_patterns = [ | |
| r'except\s+(RuntimeError|KeyError|ValueError|Exception)', | |
| ] | |
| found_error_handling = False | |
| for try_pattern in state_dict_load_patterns: | |
| for match in re.finditer(try_pattern, content, re.MULTILINE): | |
| # Find the corresponding except block after the try block | |
| try_block_end = match.end() | |
| except_match = re.search(exception_patterns[0], content[try_block_end:]) | |
| if except_match: | |
| found_error_handling = True | |
| break | |
| if found_error_handling: | |
| break | |
| error_handling_patterns = [found_error_handling] |
| if passed >= 3: # Allow for additional improvements to be optional | ||
| print("\n🎉 ALL REQUIRED CODE REVIEW COMMENTS SUCCESSFULLY ADDRESSED!") | ||
| print("\n📋 Summary of fixes implemented:") | ||
| print(" ✅ Comment 1: Hardcoded absolute paths → Environment variable configuration") | ||
| print(" ✅ Comment 2: Interactive login issues → Non-interactive environment detection") | ||
| print(" ✅ Comment 3: No state dict error handling → Comprehensive error handling") | ||
| print(" ✅ Bonus: Enhanced authentication, PyTorch compatibility, better error messages") | ||
|
|
||
| return True | ||
| print(f"\n⚠️ Only {passed}/{total} validations passed - some fixes may need review") | ||
| return False |
There was a problem hiding this comment.
suggestion (bug_risk): Threshold for passing validations may be too lenient.
Permitting a single required validation to fail may allow critical issues to slip through. Recommend requiring all mandatory validations to pass, or explicitly separating required from optional checks.
| if passed >= 3: # Allow for additional improvements to be optional | |
| print("\n🎉 ALL REQUIRED CODE REVIEW COMMENTS SUCCESSFULLY ADDRESSED!") | |
| print("\n📋 Summary of fixes implemented:") | |
| print(" ✅ Comment 1: Hardcoded absolute paths → Environment variable configuration") | |
| print(" ✅ Comment 2: Interactive login issues → Non-interactive environment detection") | |
| print(" ✅ Comment 3: No state dict error handling → Comprehensive error handling") | |
| print(" ✅ Bonus: Enhanced authentication, PyTorch compatibility, better error messages") | |
| return True | |
| print(f"\n⚠️ Only {passed}/{total} validations passed - some fixes may need review") | |
| return False | |
| if passed == total: | |
| print("\n🎉 ALL REQUIRED CODE REVIEW COMMENTS SUCCESSFULLY ADDRESSED!") | |
| print("\n📋 Summary of fixes implemented:") | |
| print(" ✅ Comment 1: Hardcoded absolute paths → Environment variable configuration") | |
| print(" ✅ Comment 2: Interactive login issues → Non-interactive environment detection") | |
| print(" ✅ Comment 3: No state dict error handling → Comprehensive error handling") | |
| print(" ✅ Bonus: Enhanced authentication, PyTorch compatibility, better error messages") | |
| return True | |
| print(f"\n⚠️ Only {passed}/{total} validations passed - some fixes may need review") | |
| return False |
| if "os.makedirs(test_projects_dir, exist_ok=True)" in content: | ||
| print(" ✅ Creates actual directory structure before testing") | ||
|
|
||
| # Check that it validates directory existence | ||
| if "os.path.exists" in content: | ||
| print(" ✅ Validates directory existence in output") | ||
| return True |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if "os.path.exists" in content: | ||
| print(" ✅ Validates directory existence in output") | ||
| return True |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if not os.path.exists(api_server): | ||
| print(f" ❌ API server not found: {api_server}") | ||
| return False |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for pattern in timeout_patterns: | ||
| if pattern in content: | ||
| timeout_checks.append(f"Contains: {pattern}") |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if pattern in content: | ||
| timeout_checks.append(f"Contains: {pattern}") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Resolved issues in scripts/deployment/test_code_review_fixes_v3.py with DeepSource Autofix
There was a problem hiding this comment.
Code Review
This pull request introduces three new scripts for testing and validation of code review fixes. The scripts are generally well-structured, but there are a few areas for improvement. My review focuses on improving the correctness and robustness of the tests. Specifically, I've pointed out a flawed test for tilde path expansion, suggested improvements to a regex test to make it more accurate, and recommended a change to make a validation script more portable by not relying on hardcoded paths. These changes will make the new test suite more reliable and maintainable.
| print("\n🏠 Test 4: With home directory path expansion") | ||
|
|
||
| # Create a temporary directory under the expanded home path to ensure it exists | ||
| with tempfile.TemporaryDirectory() as temp_base: | ||
| # Set up the directory structure | ||
| test_projects_dir = os.path.join(temp_base, "Projects", "SAMO-DL") | ||
| os.makedirs(test_projects_dir, exist_ok=True) | ||
|
|
||
| # Set the environment variable with tilde form | ||
| tilde_path = f"~{temp_base.replace(os.path.expanduser('~'), '')}/Projects/SAMO-DL" | ||
| os.environ['SAMO_DL_BASE_DIR'] = tilde_path | ||
|
|
||
| detected_path = get_model_base_directory() | ||
| expected_path = os.path.join(test_projects_dir, "deployment", "models") | ||
|
|
||
| print(f" Environment var: {os.getenv('SAMO_DL_BASE_DIR')}") | ||
| print(f" Detected path: {detected_path}") | ||
| print(f" Expected: {expected_path}") | ||
| print(f" Match: {detected_path == expected_path}") | ||
| print(f" Directory exists: {os.path.exists(os.path.dirname(detected_path))}") |
There was a problem hiding this comment.
The logic in "Test 4: With home directory path expansion" for testing tilde (~) expansion is flawed. The construction of tilde_path will not work correctly if the temporary directory is not created inside the user's home directory (which is common for /tmp). For example, if temp_base is /tmp/foo and home is /home/user, tilde_path becomes ~/tmp/foo/Projects/SAMO-DL, which expands to /home/user/tmp/foo/Projects/SAMO-DL, not /tmp/foo/Projects/SAMO-DL. This means detected_path will not match expected_path.
A more robust way to test this is to mock os.path.expanduser to control the expansion behavior without relying on where tempfile creates directories.
print("\n🏠 Test 4: With home directory path expansion")
from unittest.mock import patch
with tempfile.TemporaryDirectory() as temp_dir:
# Use a fake home directory within the temporary directory
fake_home = temp_dir
test_projects_dir = os.path.join(fake_home, "Projects", "SAMO-DL")
os.makedirs(test_projects_dir, exist_ok=True)
# Patch os.path.expanduser to use our fake home
with patch('os.path.expanduser', side_effect=lambda p: p.replace('~', fake_home)):
os.environ['SAMO_DL_BASE_DIR'] = "~/Projects/SAMO-DL"
detected_path = get_model_base_directory()
expected_path = os.path.join(test_projects_dir, "deployment", "models")
print(f" Environment var: {os.getenv('SAMO_DL_BASE_DIR')}")
print(f" Detected path: {detected_path}")
print(f" Expected: {expected_path}")
print(f" Match: {detected_path == expected_path}")
print(f" Directory exists: {os.path.exists(os.path.dirname(detected_path))}")| print("🔧 Testing regex-based model replacement functionality...") | ||
|
|
||
| # Simulate the regex patterns from our fixed code | ||
| tokenizer_pattern = r'AutoTokenizer\.from_pretrained\s*\(\s*[\'"][^\'\"]+[\'"]\s*\)' | ||
| model_pattern = r'AutoModelForSequenceClassification\.from_pretrained\s*\(\s*[\'"][^\'\"]+[\'"]' | ||
|
|
||
| # Test various formatting scenarios | ||
| test_cases = [ | ||
| # Standard formatting | ||
| "AutoTokenizer.from_pretrained('distilroberta-base')", | ||
| "AutoTokenizer.from_pretrained(\"distilroberta-base\")", | ||
|
|
||
| # Extra whitespace | ||
| "AutoTokenizer.from_pretrained( 'distilroberta-base' )", | ||
| "AutoTokenizer.from_pretrained(\n 'distilroberta-base'\n)", | ||
|
|
||
| # Model cases | ||
| "AutoModelForSequenceClassification.from_pretrained('distilroberta-base'", | ||
| "AutoModelForSequenceClassification.from_pretrained( \"distilroberta-base\"", | ||
| ] | ||
|
|
||
| repo_name = "user/test-model" | ||
| tokenizer_replacement = f"AutoTokenizer.from_pretrained('{repo_name}')" | ||
| model_replacement = f"AutoModelForSequenceClassification.from_pretrained('{repo_name}'" | ||
|
|
||
| successes = 0 | ||
| for i, test_case in enumerate(test_cases): | ||
| print(f" Test case {i+1}: {test_case[:50]}...") | ||
|
|
||
| if "AutoTokenizer" in test_case: | ||
| result = re.sub(tokenizer_pattern, tokenizer_replacement, test_case) | ||
| expected = tokenizer_replacement | ||
| else: | ||
| result = re.sub(model_pattern, model_replacement, test_case) | ||
| expected = model_replacement | ||
|
|
||
| if expected in result: | ||
| print(" ✅ Regex replacement successful") | ||
| successes += 1 | ||
| else: | ||
| print(f" ❌ Regex replacement failed: {result}") | ||
|
|
||
| print(f" Regex patterns successful: {successes}/{len(test_cases)}") | ||
| return successes == len(test_cases) | ||
|
|
||
| def test_config_file_creation(): |
There was a problem hiding this comment.
This test function can be improved in two ways for better correctness:
- The
model_patternand its associated test cases and replacement string are missing the closing parenthesis of thefrom_pretrained()call, making them inconsistent with thetokenizer_patternand likely not matching the real code. - The check
if expected in resultis too lenient. It would pass for partial matches (e.g., if extra characters were appended to the result). Usingresult == expectedensures an exact match.
Here is a suggested implementation that addresses both points:
def test_regex_based_model_replacement():
"""Test that the model replacement now uses robust regex patterns."""
print("🔧 Testing regex-based model replacement functionality...")
# Simulate the regex patterns from our fixed code
tokenizer_pattern = r'AutoTokenizer\.from_pretrained\s*\(\s*[\'"][^\'"]+[\'"]\s*\)'
model_pattern = r'AutoModelForSequenceClassification\.from_pretrained\s*\(\s*[\'"][^\'"]+[\'"]\s*\)'
# Test various formatting scenarios
test_cases = [
# Standard formatting
"AutoTokenizer.from_pretrained('distilroberta-base')",
"AutoTokenizer.from_pretrained(\"distilroberta-base\")",
# Extra whitespace
"AutoTokenizer.from_pretrained( 'distilroberta-base' )",
"AutoTokenizer.from_pretrained(\n 'distilroberta-base'\n)",
# Model cases
"AutoModelForSequenceClassification.from_pretrained('distilroberta-base')",
"AutoModelForSequenceClassification.from_pretrained( \"distilroberta-base\" )",
]
repo_name = "user/test-model"
tokenizer_replacement = f"AutoTokenizer.from_pretrained('{repo_name}')"
model_replacement = f"AutoModelForSequenceClassification.from_pretrained('{repo_name}')"
successes = 0
for i, test_case in enumerate(test_cases):
print(f" Test case {i+1}: {test_case[:50]}...")
if "AutoTokenizer" in test_case:
result = re.sub(tokenizer_pattern, tokenizer_replacement, test_case)
expected = tokenizer_replacement
else:
result = re.sub(model_pattern, model_replacement, test_case)
expected = model_replacement
if result == expected:
print(f" ✅ Regex replacement successful")
successes += 1
else:
print(f" ❌ Regex replacement failed. Got: '{result}'")
print(f" Regex patterns successful: {successes}/{len(test_cases)}")
return successes == len(test_cases)| import sys | ||
|
|
||
| def validate_comment_1_portability(): | ||
| """Validate that Comment 1 (hardcoded paths) has been addressed.""" | ||
| print("🧪 VALIDATING PORTABILITY FIX (Comment 1)") | ||
| print("=" * 50) | ||
|
|
||
| script_path = "scripts/deployment/upload_model_to_huggingface.py" |
There was a problem hiding this comment.
The script hardcodes the path to upload_model_to_huggingface.py inside each validation function (e.g., line 14, 68, 129, 190), assuming the script is run from the repository root. This is brittle and repetitive.
To make the script more robust and maintainable, you should define the path as a constant at the module level, constructing it relative to this script's location. This allows the script to be run from any directory.
Please apply this pattern to all functions that use script_path.
import sys
import os
# Path to the script being validated, relative to this script's location.
SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
REPO_ROOT = os.path.abspath(os.path.join(SCRIPT_DIR, '..', '..'))
UPLOAD_SCRIPT_PATH = os.path.join(REPO_ROOT, 'scripts/deployment/upload_model_to_huggingface.py')
def validate_comment_1_portability():
"""Validate that Comment 1 (hardcoded paths) has been addressed."""
print("🧪 VALIDATING PORTABILITY FIX (Comment 1)")
print("=" * 50)
script_path = UPLOAD_SCRIPT_PATH![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
scripts/deployment/test_model_path_detection.py (1)
78-78: Fix tempfile usage mismatch (import vs usage).You're using
tempfile.TemporaryDirectory()at Line 78 but only importTemporaryDirectory(Line 39). Two safe options:
- Minimal change (preferred here): use the already imported symbol.
- with tempfile.TemporaryDirectory() as temp_base: + with TemporaryDirectory() as temp_base:
- Alternative: add a top-level
import tempfileand keep Line 78 as-is.Also applies to: 39-41
scripts/deployment/validate_code_review_fixes.py (5)
27-37: Make hardcoded path policy stricter (or prod-aware).Allowing one hardcoded path can mask issues. Either prohibit hardcoded paths in production or explicitly allow only in test code as per prior feedback.
- # Check for hardcoded paths (should be minimal/none) - hardcoded_indicators = [ - content.count('/Users/') <= 1, # Allow one or fewer hardcoded /Users/ paths - content.count('/home/') <= 1, # Allow one or fewer hardcoded /home/ paths - 'configurable' in content.lower(), - 'environment variable' in content.lower() - ] - - all_env_vars = all(env_vars_found) - no_hardcoded = all(hardcoded_indicators) + # Check for hardcoded paths (none in production code) + is_test_code = ( + 'test' in script_path.lower() + or 'unittest' in content.lower() + or 'pytest' in content.lower() + ) + hardcoded_users_count = content.count('/Users/') + hardcoded_home_count = content.count('/home/') + + all_env_vars = all(env_vars_found) + if not is_test_code: + no_hardcoded = (hardcoded_users_count == 0 and hardcoded_home_count == 0) + else: + # Allow hardcoded paths in test code + no_hardcoded = True
74-82: Detect non-interactive environments with explicit patterns, not substring heuristics.Substrings like 'CI'/'DOCKER' can match comments/strings. Use regex to detect actual environment checks or isatty calls.
- # Check for non-interactive environment detection - interactive_checks = [ - 'is_interactive_environment' in content, - 'CI' in content and 'DOCKER' in content, # Environment checks - 'KUBERNETES' in content, - 'sys.stdin.isatty()' in content, - 'non-interactive' in content.lower() - ] + # Check for explicit non-interactive environment detection + import re + env_var_patterns = [ + r'os\.environ\.get\(["\']CI["\']\)', + r'os\.environ\.get\(["\']DOCKER["\']\)', + r'os\.environ\.get\(["\']KUBERNETES["\']\)', + r'os\.getenv\(["\']CI["\']\)', + r'os\.getenv\(["\']DOCKER["\']\)', + r'os\.getenv\(["\']KUBERNETES["\']\)', + r'if\s+["\']CI["\']\s+in\s+os\.environ', + r'if\s+["\']DOCKER["\']\s+in\s+os\.environ', + r'if\s+["\']KUBERNETES["\']\s+in\s+os\.environ', + r'sys\.stdin\.isatty\(\)' + ] + interactive_checks = [ + any(re.search(p, content) for p in env_var_patterns), + 'is_interactive_environment' in content, + 'sys.stdin.isatty()' in content, + ]
92-97: Refine consent detection to avoid false positives.A generic
'input('match can flag unrelated prompts. Search for explicit consent prompts tied to interactive login.- # Check for user consent before interactive login - user_consent_checks = [ - 'input(' in content, # User input for consent - 'Attempt interactive login' in content, - 'y/N' in content or 'yes/no' in content - ] + # Check for explicit user consent prompts tied to login attempts + user_consent_checks = [ + any( + kw in content + for kw in [ + "input('Do you consent", + 'input("Do you consent', + "input('Do you agree", + 'input("Do you agree', + "input('Allow interactive login", + 'input("Allow interactive login', + "input('Grant permission", + 'input("Grant permission', + ] + ), + "interactive login" in content and ( + "consent" in content or "agree" in content or "permission" in content + ), + ]
135-143: Tie error handling detection to actual state-dict load try/except blocks.Presence of 'try'/'except' substrings is weak. Look for try blocks that call
torch.load/model.load_state_dictwith corresponding except clauses.- # Check for error handling around state dict loading - error_handling_patterns = [ - 'try:' in content and 'except' in content, - 'RuntimeError' in content, - 'size mismatch' in content, - 'KeyError' in content, - 'Architecture mismatch' in content - ] + # Improved detection: exception blocks directly handling state dict loading + import re + state_dict_load_patterns = [ + r'try\s*:\s*\n(?:[^\n]*\n)*?\s*(torch\.load|model\.load_state_dict)\s*\(', + ] + exception_patterns = [ + r'except\s+(RuntimeError|KeyError|ValueError|Exception)', + ] + found_error_handling = False + for try_pattern in state_dict_load_patterns: + for match in re.finditer(try_pattern, content, re.MULTILINE): + try_block_end = match.end() + except_match = re.search(exception_patterns[0], content[try_block_end:]) + if except_match: + found_error_handling = True + break + if found_error_handling: + break @@ - has_error_handling = sum(error_handling_patterns) >= 4 + has_error_handling = found_error_handlingAlso applies to: 152-159, 160-163
259-269: Require all mandatory validations to pass; keep “Additional Improvements” optional.Allowing 1 required validation to fail is risky. Ensure the first three validators pass; treat the bonus separately.
- if passed >= 3: # Allow for additional improvements to be optional + # All required checks must pass; additional improvements are optional + required = [result for (name, result) in results if name != "Additional Improvements"] + if all(required):scripts/deployment/test_code_review_fixes_v3.py (2)
201-213: Detect imports via AST, not string counts.String matching may hit comments/strings; AST ensures accuracy.
- # Check that duplicate sys import is removed - sys_import_count = content.count("import sys") - if sys_import_count <= 1: # Should be 0 now, but allow 1 for safety - improvements.append("Duplicate sys import removed") + # Check that duplicate sys import is removed (AST-based) + sys_import_count = 0 + try: + tree = ast.parse(content) + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "sys": + sys_import_count += 1 + except Exception as e: + print(f" ❌ AST parsing error: {e}") + return False + if sys_import_count <= 1: + improvements.append("Duplicate sys import removed")
298-329: Use regex to find PII exposures in error contexts and remove unused variable.Leverage regex to scan for
"text": textaround error contexts; also remove the unusederror_patterns.- # Find all error response patterns - error_patterns = [ - r'"error":\s*[^}]+\}', # Error responses - r'"error":[^,}]+,', # Error fields in responses - ] - - pii_exposures = [] - - # Look for "text": text in error contexts - lines = content.split('\n') - for i, line in enumerate(lines): - if '"text": text' in line: - # Check surrounding context for error indicators - context_start = max(0, i-5) - context_end = min(len(lines), i+5) - context = ' '.join(lines[context_start:context_end]) - - if any(error_indicator in context.lower() for error_indicator in ['error', 'exception', 'failed', 'timeout']): - # Check if this is actually a successful response (should contain emotion) - if '"emotion"' not in context: - pii_exposures.append(f"Line {i+1}: {line.strip()}") + pii_exposures = [] + # Use regex to find all occurrences of '"text": text' and inspect surrounding context + for match in re.finditer(r'"text":\s*text', content): + start_idx = max(0, match.start() - 500) + end_idx = min(len(content), match.end() + 500) + context = content[start_idx:end_idx] + if re.search(r'error|exception|failed|timeout', context, re.IGNORECASE): + # If this isn’t a successful response (no "emotion" field nearby), flag it + if not re.search(r'"emotion"', context): + line_num = content[:match.start()].count('\n') + 1 + pii_exposures.append(f"Line {line_num}: {match.group().strip()}")
🧹 Nitpick comments (13)
scripts/deployment/test_model_path_detection.py (4)
12-15: Avoid sys.path mutation; import module robustly.Module-level sys.path tweaks can be brittle and trigger E402. Prefer importlib to load sibling script without permanent path mutation.
Example:
import importlib.util target = os.path.join(script_dir, "upload_model_to_huggingface.py") spec = importlib.util.spec_from_file_location("upload_model_to_hf", target) mod = importlib.util.module_from_spec(spec) spec.loader.exec_module(mod) get_model_base_directory = mod.get_model_base_directory
35-35: Prefer pathlib over os.path utilities for clarity and cross-platform robustness.Ruff flags multiple spots where
PathAPI would be cleaner (exists,join,mkdir,expanduser).If you adopt Pathlib, consistently:
- Build paths via
Path(...) / "deployment" / "models"- Use
Path.exists(),Path.mkdir(parents=True, exist_ok=True)- Expand
~withPath("~").expanduser()Also applies to: 44-45, 63-64, 80-81, 84-84, 88-88, 94-94
91-91: Trailing whitespace.Remove trailing whitespace to satisfy linters.
114-114: Add trailing newline at EOF.Many tools expect a newline at end of file.
scripts/deployment/validate_code_review_fixes.py (2)
17-17: Remove unnecessary 'r' in open(); default mode is read.Cleans up per Ruff UP015 without changing behavior.
- with open(script_path, 'r') as f: + with open(script_path) as f:Repeat similarly at Lines 71, 132, and 193.
Also applies to: 71-71, 132-132, 193-193
220-220: Ambiguous ℹ character flagged by linter.If your tooling flags it (RUF001), consider replacing with plain 'i' or omitting the symbol.
scripts/deployment/test_code_review_fixes_v3.py (7)
160-166: Iterate dict directly; avoid .keys().Use
for key in id2label:instead ofid2label.keys().- for key in id2label.keys(): + for key in id2label:
195-195: Remove unnecessary 'r' in open(); default is read.Cleans up per Ruff UP015.
- with open(upload_script, 'r') as f: + with open(upload_script) as f:Apply similarly at Lines 237, 265, 295, and 354.
Also applies to: 237-237, 265-265, 295-295, 354-354
74-74: Consider Pathlib for filesystem ops (clarity and cross-platform).Ruff flags multiple
os.path/os.makedirsusages that Pathlib can simplify. Optional, but improves readability.Also applies to: 78-78, 90-90, 233-233, 261-261, 291-291, 349-349
110-110: Remove trailing whitespace flagged by linters.Clean up lines with W291.
Also applies to: 125-125, 423-423
435-435: Add trailing newline at EOF.Satisfies W292 and common tooling expectations.
26-48: Regex for model replacement: consider ensuring closing parenthesis handling.Current
model_patternexcludes the closing)to fit partial replacements. Ensure downstream substitution won’t leave unmatched parentheses in real code paths.
240-252: Presence-only test may miss correctness bugs.
test_path_expansion_fixchecks for string presence (TemporaryDirectory, makedirs, exists) rather than executing the logic. It won’t catch broken tilde expansion in the other script.We can import and run the function under a temporary HOME to validate behavior end-to-end.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/deployment/test_code_review_fixes_v3.py(1 hunks)scripts/deployment/test_model_path_detection.py(1 hunks)scripts/deployment/validate_code_review_fixes.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
scripts/deployment/validate_code_review_fixes.py
17-17: Unnecessary mode argument
Remove mode argument
(UP015)
71-71: Unnecessary mode argument
Remove mode argument
(UP015)
132-132: Unnecessary mode argument
Remove mode argument
(UP015)
193-193: Unnecessary mode argument
Remove mode argument
(UP015)
220-220: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
scripts/deployment/test_code_review_fixes_v3.py
74-74: os.path.join() should be replaced by Path with / operator
(PTH118)
78-78: os.makedirs() should be replaced by Path.mkdir(parents=True)
(PTH103)
90-90: os.path.exists() should be replaced by Path.exists()
(PTH110)
91-91: Unnecessary mode argument
Remove mode argument
(UP015)
110-110: Trailing whitespace
Remove trailing whitespace
(W291)
125-125: Trailing whitespace
Remove trailing whitespace
(W291)
161-161: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
191-191: os.path.exists() should be replaced by Path.exists()
(PTH110)
195-195: Unnecessary mode argument
Remove mode argument
(UP015)
233-233: os.path.exists() should be replaced by Path.exists()
(PTH110)
237-237: Unnecessary mode argument
Remove mode argument
(UP015)
261-261: os.path.exists() should be replaced by Path.exists()
(PTH110)
265-265: Unnecessary mode argument
Remove mode argument
(UP015)
291-291: os.path.exists() should be replaced by Path.exists()
(PTH110)
295-295: Unnecessary mode argument
Remove mode argument
(UP015)
299-299: Local variable error_patterns is assigned to but never used
Remove assignment to unused variable error_patterns
(F841)
349-349: os.path.exists() should be replaced by Path.exists()
(PTH110)
354-354: Unnecessary mode argument
Remove mode argument
(UP015)
423-423: Trailing whitespace
Remove trailing whitespace
(W291)
435-435: No newline at end of file
Add trailing newline
(W292)
scripts/deployment/test_model_path_detection.py
12-12: os.path.abspath() should be replaced by Path.resolve()
(PTH100)
15-15: Module level import not at top of file
(E402)
35-35: os.path.exists() should be replaced by Path.exists()
(PTH110)
44-44: os.path.join() should be replaced by Path with / operator
(PTH118)
63-63: os.path.join() should be replaced by Path with / operator
(PTH118)
78-78: Undefined name tempfile
(F821)
80-80: os.path.join() should be replaced by Path with / operator
(PTH118)
81-81: os.makedirs() should be replaced by Path.mkdir(parents=True)
(PTH103)
84-84: os.path.expanduser() should be replaced by Path.expanduser()
(PTH111)
88-88: os.path.join() should be replaced by Path with / operator
(PTH118)
91-91: Trailing whitespace
Remove trailing whitespace
(W291)
94-94: os.path.exists() should be replaced by Path.exists()
(PTH110)
114-114: No newline at end of file
Add trailing newline
(W292)
| # Test 4: With expanduser (~) path | ||
| print("\n🏠 Test 4: With home directory path expansion") | ||
|
|
||
| # Create a temporary directory under the expanded home path to ensure it exists | ||
| with tempfile.TemporaryDirectory() as temp_base: | ||
| # Set up the directory structure | ||
| test_projects_dir = os.path.join(temp_base, "Projects", "SAMO-DL") | ||
| os.makedirs(test_projects_dir, exist_ok=True) | ||
|
|
||
| # Set the environment variable with tilde form | ||
| tilde_path = f"~{temp_base.replace(os.path.expanduser('~'), '')}/Projects/SAMO-DL" | ||
| os.environ['SAMO_DL_BASE_DIR'] = tilde_path | ||
|
|
||
| detected_path = get_model_base_directory() | ||
| expected_path = os.path.join(test_projects_dir, "deployment", "models") | ||
|
|
||
| print(f" Environment var: {os.getenv('SAMO_DL_BASE_DIR')}") | ||
| print(f" Detected path: {detected_path}") | ||
| print(f" Expected: {expected_path}") | ||
| print(f" Match: {detected_path == expected_path}") | ||
| print(f" Directory exists: {os.path.exists(os.path.dirname(detected_path))}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test 4 tilde expansion is incorrect; simulate HOME to make it deterministic.
Constructing tilde_path via f"~{temp_base.replace(os.path.expanduser('~'), '')}/..." is flawed. If temp_base isn’t under $HOME, the resulting string resembles ~tmp/... which expands to another user’s home or not at all, causing false negatives.
Use a temporary HOME to make ~ expansion resolve into an isolated, predictable directory.
- # Test 4: With expanduser (~) path
- print("\n🏠 Test 4: With home directory path expansion")
-
- # Create a temporary directory under the expanded home path to ensure it exists
- with tempfile.TemporaryDirectory() as temp_base:
- # Set up the directory structure
- test_projects_dir = os.path.join(temp_base, "Projects", "SAMO-DL")
- os.makedirs(test_projects_dir, exist_ok=True)
-
- # Set the environment variable with tilde form
- tilde_path = f"~{temp_base.replace(os.path.expanduser('~'), '')}/Projects/SAMO-DL"
- os.environ['SAMO_DL_BASE_DIR'] = tilde_path
-
- detected_path = get_model_base_directory()
- expected_path = os.path.join(test_projects_dir, "deployment", "models")
-
- print(f" Environment var: {os.getenv('SAMO_DL_BASE_DIR')}")
- print(f" Detected path: {detected_path}")
- print(f" Expected: {expected_path}")
- print(f" Match: {detected_path == expected_path}")
- print(f" Directory exists: {os.path.exists(os.path.dirname(detected_path))}")
+ # Test 4: With expanduser (~) path
+ print("\n🏠 Test 4: With home directory path expansion")
+ with TemporaryDirectory() as temp_home:
+ original_home = os.environ.get('HOME')
+ try:
+ # Simulate HOME so ~ expands into an isolated temp directory
+ os.environ['HOME'] = temp_home
+
+ # Create the expected project directory under the simulated HOME
+ test_projects_dir = os.path.join(temp_home, "Projects", "SAMO-DL")
+ os.makedirs(test_projects_dir, exist_ok=True)
+
+ # Set the environment variable using a plain tilde path
+ tilde_path = os.path.join("~", "Projects", "SAMO-DL")
+ os.environ['SAMO_DL_BASE_DIR'] = tilde_path
+
+ detected_path = get_model_base_directory()
+ expected_path = os.path.join(test_projects_dir, "deployment", "models")
+
+ print(f" Environment var: {os.getenv('SAMO_DL_BASE_DIR')}")
+ print(f" Detected path: {detected_path}")
+ print(f" Expected: {expected_path}")
+ print(f" Match: {detected_path == expected_path}")
+ print(f" Directory exists: {os.path.exists(os.path.dirname(detected_path))}")
+ finally:
+ if original_home is not None:
+ os.environ['HOME'] = original_home
+ elif 'HOME' in os.environ:
+ del os.environ['HOME']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Test 4: With expanduser (~) path | |
| print("\n🏠 Test 4: With home directory path expansion") | |
| # Create a temporary directory under the expanded home path to ensure it exists | |
| with tempfile.TemporaryDirectory() as temp_base: | |
| # Set up the directory structure | |
| test_projects_dir = os.path.join(temp_base, "Projects", "SAMO-DL") | |
| os.makedirs(test_projects_dir, exist_ok=True) | |
| # Set the environment variable with tilde form | |
| tilde_path = f"~{temp_base.replace(os.path.expanduser('~'), '')}/Projects/SAMO-DL" | |
| os.environ['SAMO_DL_BASE_DIR'] = tilde_path | |
| detected_path = get_model_base_directory() | |
| expected_path = os.path.join(test_projects_dir, "deployment", "models") | |
| print(f" Environment var: {os.getenv('SAMO_DL_BASE_DIR')}") | |
| print(f" Detected path: {detected_path}") | |
| print(f" Expected: {expected_path}") | |
| print(f" Match: {detected_path == expected_path}") | |
| print(f" Directory exists: {os.path.exists(os.path.dirname(detected_path))}") | |
| # Test 4: With expanduser (~) path | |
| print("\n🏠 Test 4: With home directory path expansion") | |
| with TemporaryDirectory() as temp_home: | |
| original_home = os.environ.get('HOME') | |
| try: | |
| # Simulate HOME so ~ expands into an isolated temp directory | |
| os.environ['HOME'] = temp_home | |
| # Create the expected project directory under the simulated HOME | |
| test_projects_dir = os.path.join(temp_home, "Projects", "SAMO-DL") | |
| os.makedirs(test_projects_dir, exist_ok=True) | |
| # Set the environment variable using a plain tilde path | |
| tilde_path = os.path.join("~", "Projects", "SAMO-DL") | |
| os.environ['SAMO_DL_BASE_DIR'] = tilde_path | |
| detected_path = get_model_base_directory() | |
| expected_path = os.path.join(test_projects_dir, "deployment", "models") | |
| print(f" Environment var: {os.getenv('SAMO_DL_BASE_DIR')}") | |
| print(f" Detected path: {detected_path}") | |
| print(f" Expected: {expected_path}") | |
| print(f" Match: {detected_path == expected_path}") | |
| print(f" Directory exists: {os.path.exists(os.path.dirname(detected_path))}") | |
| finally: | |
| if original_home is not None: | |
| os.environ['HOME'] = original_home | |
| elif 'HOME' in os.environ: | |
| del os.environ['HOME'] |
🧰 Tools
🪛 Ruff (0.12.2)
78-78: Undefined name tempfile
(F821)
80-80: os.path.join() should be replaced by Path with / operator
(PTH118)
81-81: os.makedirs() should be replaced by Path.mkdir(parents=True)
(PTH103)
84-84: os.path.expanduser() should be replaced by Path.expanduser()
(PTH111)
88-88: os.path.join() should be replaced by Path with / operator
(PTH118)
91-91: Trailing whitespace
Remove trailing whitespace
(W291)
94-94: os.path.exists() should be replaced by Path.exists()
(PTH110)
🤖 Prompt for AI Agents
In scripts/deployment/test_model_path_detection.py around lines 74 to 95, the
test for tilde (~) expansion constructs the tilde_path incorrectly by appending
a modified temp_base to '~', which can lead to invalid expansions if temp_base
is not under the actual HOME directory. To fix this, set the HOME environment
variable to temp_base during the test so that '~' expands predictably within the
isolated temporary directory, ensuring the test is deterministic and does not
depend on the real user's home directory.
Summary by Sourcery
Add three new deployment scripts: a V3 test suite for end-to-end code review fixes, a validator to check that specific code review comments have been implemented, and a portable model path detection test to verify environment variable and path expansion logic.
New Features:
Tests:
Summary by CodeRabbit
New Features
Tests