Fix NoneType errors and simplify proxy conditionals#29
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughGPU validation defaults now use explicit empty strings for memory page size and SSH proxy command. Task conditions use direct values, while proxy setup and service templates support either HTTP or HTTPS proxy values with HTTP-to-HTTPS fallback. ChangesGPU validation configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
9febde5 to
5c0727b
Compare
Set empty string defaults ('') for gpu_validation_ssh_proxy_command
and gpu_validation_mem_page_size to prevent NoneType errors when
used with Jinja2 length filter or string interpolation. Remove
redundant default('') calls at usage sites.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5c0727b to
afb811c
Compare
Empty strings and None are both falsy in Jinja2, so the verbose
'| length > 0' pattern is unnecessary for string variables.
Replaced with simple '{% if var %}' checks in setup.yaml,
vllm-serve.service.j2, and vm_image.yaml.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@gpu-validation/tasks/setup.yaml`:
- Line 11: Update the guards in gpu-validation/tasks/setup.yaml at lines 11-11
and gpu-validation/templates/vllm-serve.service.j2 at lines 8-8 and 23-23 to use
gpu_validation_http_proxy or gpu_validation_https_proxy. This must include
proxy.yaml and emit both host and container proxy variables whenever either
proxy configuration is set.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d4be2836-0bcf-4920-a73b-b7f6972ed289
📒 Files selected for processing (3)
gpu-validation/tasks/setup.yamlgpu-validation/tasks/vm_image.yamlgpu-validation/templates/vllm-serve.service.j2
🚧 Files skipped from review as they are similar to previous changes (1)
- gpu-validation/tasks/vm_image.yaml
The proxy guards only checked gpu_validation_http_proxy, ignoring HTTPS-only setups. Updated all guards to trigger on either proxy being set and added bidirectional fallback so HTTP falls back to HTTPS and vice versa. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Action performedReview finished.
|
What does this PR do?
default('')calls at usage sites.| length > 0to truthiness test.Why do we need this PR?
NoneTypeerrors when used with Jinja2 length filter or string interpolation.| length > 0pattern is unnecessary for string variables.gpu_validation_http_proxy, ignoring HTTPS-only setups.