Skip to content

fix(gcp): remove unsupported --include flags from gcloud storage rsync#799

Open
arniechops wants to merge 14 commits intomainfrom
arnavchopra/fix-gcs-rsync-include-flags
Open

fix(gcp): remove unsupported --include flags from gcloud storage rsync#799
arniechops wants to merge 14 commits intomainfrom
arnavchopra/fix-gcs-rsync-include-flags

Conversation

@arniechops
Copy link
Copy Markdown
Collaborator

@arniechops arniechops commented Apr 3, 2026

Summary

  • gcloud storage rsync only supports --exclude (Python regex), not --include
  • The --include flags caused model weight downloads to fail on GCP with: unrecognized arguments: --include=*.model
  • Removed --include flags and kept only --exclude="optimizer.*" since all other files in the checkpoint bucket are needed

Test plan

  • Deploy model-engine with this fix on GCP cluster
  • Create an endpoint and verify vLLM container successfully downloads weights from GCS
  • Verify optimizer files are still excluded

🤖 Generated with Claude Code

Greptile Summary

This PR fixes a GCP model weight download failure by replacing unsupported --include flags with the supported --exclude-only approach in gcloud storage rsync. The previous code passed multiple --include="*.model", --include="*.json", etc. flags that gcloud storage rsync does not recognise, causing hard failures with unrecognized arguments.

Key changes:

  • Replaced the broken --include allowlist with an --exclude denylist in load_model_weights_sub_commands_gcs
  • optimizer.* files are always excluded (Python regex, consistent with gcloud storage rsync semantics)
  • .py files are excluded via --exclude=".*\\.py$" when trust_remote_code=False, preserving the security guard that prevents arbitrary code from being pulled to the pod
  • When trust_remote_code=True, only optimizer files are excluded — .py files are downloaded as intended
  • Unit tests updated to assert the corrected --exclude-only command construction for both trust_remote_code paths

The trust_remote_code security boundary raised in a previous review thread has been fully addressed by the conditional --exclude=".*\\.py$" logic.

Confidence Score: 5/5

Safe to merge — the fix is minimal, targeted, and correct with no remaining blocking issues.

All P1 concerns from prior review threads have been addressed. The trust_remote_code security boundary is preserved. Both code paths are covered by updated unit tests. No new logic issues introduced.

No files require special attention.

Important Files Changed

Filename Overview
model-engine/model_engine_server/domain/use_cases/llm_model_endpoint_use_cases.py Replaces unsupported --include allowlist with --exclude denylist; trust_remote_code security boundary preserved via conditional .py exclusion
model-engine/tests/unit/domain/test_llm_use_cases.py Tests updated to assert new --exclude-only rsync flags for both trust_remote_code=True and False cases

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[load_model_weights_sub_commands_gcs] --> B[Install gcloud CLI via curl]
    B --> C["excludes = ['--exclude=\"optimizer.*\"']
Always exclude optimizer files"]
    C --> D{trust_remote_code?}
    D -- False --> E["Append '--exclude=\".*\\.py$\"'
Block .py files from download"]
    D -- True --> F[No additional excludes
.py files allowed]
    E --> G[file_selection_str = join excludes]
    F --> G
    G --> H["gcloud storage rsync -r {excludes}
gs://bucket/checkpoint → dest_folder"]
Loading

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Reviews (3): Last reviewed commit: "fix(tests): update GCS unit tests to mat..." | Re-trigger Greptile

@arniechops arniechops enabled auto-merge (squash) April 3, 2026 01:48
@arniechops arniechops disabled auto-merge April 3, 2026 01:49
@arniechops arniechops enabled auto-merge (squash) April 5, 2026 21:30
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants