feat: add cog.Secret input support to coglet#3057
Conversation
Classify and coerce Secret, Optional[Secret], and Secret | None fields, wrapping plain string values in cog.types.Secret (list[Secret] is intentionally not coerced). Add the hello-replicate example which demonstrates Secret usage by calling the Replicate API.
- Remove coglet Secret coercion (moved to dedicated PR #3057) - Remove hello-replicate example and its test-harness entry (moved to #3057) - Drop Secret usage from the scalar-types fixture - Gate managed-weights harness run behind COG_MANAGED_WEIGHTS_READY - Fix stale predict.py/run.py and max:32/max:4 references in hello-concurrency README - Fix hello-context README to match the text input - Fix resnet README to describe GPU + torchvision setup-time weight fetch - Update resnet-managed-weights README to reference model: instead of image:
|
LGTM |
Pinned to pyo3 0.27 due to numpy/pyo3-async-runtimes lag; neither vulnerable code path is reachable in coglet.
|
@anish-sahoo Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
markphelps
left a comment
markphelps
left a comment
There was a problem hiding this comment.
Adversarial review found a few fixes worth doing before merge; comments inline.
|
|
||
| #[test] | ||
| #[ignore] // Requires cog Python package in PYTHONPATH | ||
| fn detect_direct_secret() { |
There was a problem hiding this comment.
This adds useful Secret annotation coverage, but these tests are all #[ignore] and only validate classification. The behavior this PR changes is runtime coercion in coerce_typed_inputs, so default CI still would not catch a predictor receiving a plain str instead of cog.Secret. Could you add active coglet-python pytest coverage that asserts a predictor annotated with Secret receives a cog.Secret whose get_secret_value() returns the submitted value? Ideally cover Secret, Optional[Secret], Union[Secret, None] / Secret | None, plus negative cases like non-secret str staying str and optional None staying None.
There was a problem hiding this comment.
Added active pytest coverage in TestSecretInput (Secret/Optional/PEP604 + negatives) — done in 6340d2f.
| /// Inspect a Python function's type annotations to find parameters typed as | ||
| /// `cog.File` or `cog.Path` (including `list[...]`, `Optional[...]`, | ||
| /// `... | None`, etc.). | ||
| /// `cog.File`, `cog.Path`, or `cog.Secret` (including `list[...]`, |
There was a problem hiding this comment.
This wording now overstates Secret support: File/Path support list forms, but the implementation intentionally does not classify or coerce list[Secret] (classify_type only handles list elements for File/Path). Could you narrow these comments to say Secret supports direct Secret, Optional[Secret], and Secret | None only, or otherwise make the list behavior explicit?
There was a problem hiding this comment.
Narrowed the doc comments to make explicit that list[Secret] is not classified/coerced — done in 6340d2f.
| ## Run it | ||
|
|
||
| ```sh | ||
| cog predict -i image=@cat.png -i replicate_api_token=r8_... |
There was a problem hiding this comment.
Because this example is specifically about secret handling, I’d avoid recommending a literal token on the command line: it can leak through shell history and sometimes process listings before cog.Secret ever gets a chance to redact it. Could you show a safer flow, e.g. reading from an environment variable/stdin or another non-literal-token pattern, and mention that cog.Secret redacts model logs but cannot protect secrets exposed by the user’s shell?
There was a problem hiding this comment.
Switched to an env-var flow and added a note that cog.Secret can't protect shell-exposed secrets — done in 6340d2f.
- Add active coglet-python pytest coverage asserting predictors annotated with Secret receive a cog.Secret whose get_secret_value() round-trips, covering Secret, Optional[Secret], Secret | None, plus negatives (plain str stays str, omitted Optional[Secret] stays None). - Narrow input.rs doc comments so they no longer imply list[Secret] is classified/coerced (Secret supports only direct Secret, Optional[Secret], and Secret | None). - Update hello-replicate README to recommend passing the token via an environment variable instead of a literal on the command line, and note that cog.Secret cannot protect secrets exposed by the user's shell.
* feat: add examples * format: go and python * fix: lint and format refactoring * fix: code review and update cargo deny * feat: add secret support to coglet * fix: review fixes for examples and test-harness - Document list[Secret] coercion exclusion in coglet input handling - Fix stale 'predict.py' comments in example cog.yaml files - Correct hello-train README to use 'cog predict' - Add trailing newlines to example cog.yaml files - Only pass --setup-timeout for 'cog predict' (not 'cog train') - Re-add train_tests for hello-train * feat: move resnet to experimental * refactor: split out Secret support; address example review comments - Remove coglet Secret coercion (moved to dedicated PR replicate#3057) - Remove hello-replicate example and its test-harness entry (moved to replicate#3057) - Drop Secret usage from the scalar-types fixture - Gate managed-weights harness run behind COG_MANAGED_WEIGHTS_READY - Fix stale predict.py/run.py and max:32/max:4 references in hello-concurrency README - Fix hello-context README to match the text input - Fix resnet README to describe GPU + torchvision setup-time weight fetch - Update resnet-managed-weights README to reference model: instead of image: * refactor(test-harness): base_dir-first model resolution - Resolve local models by base_dir (relative to repo root) instead of requiring 'repo: local'; precedence is base_dir, then repo, then the default fixtures/models dir. - Reject 'repo: local' in manifest.Load with a migration message. - Drop redundant 'repo: local' from all manifest entries; examples use 'base_dir: examples', fixtures use 'base_dir: tools/test-harness/fixtures/models'. - Add Model.Source() for clearer 'list' output when repo is empty. - Add tests for resolution, repo:local rejection, and Source().
|
LGTM |
2 participants
Split out from #3055 (example migration) per review feedback: coglet
Secretinput coercion is new runtime behavior and deserves its own focused review and tests.coglet
cog.Secretinput support: classify and coerceSecret,Optional[Secret], andSecret | Nonefields, wrapping plain string values incog.types.Secret.list[Secret]is intentionally not coerced.coerce_url_stringstocoerce_typed_inputsto reflect that it now handles File/Path/Secret coercion.Secretactually receive acog.Secretwhoseget_secret_value()round-trips the submitted value — coveringSecret,Optional[Secret],Secret | None, plus negatives (plainstrstaysstr, omittedOptional[Secret]staysNone).examples
hello-replicate, which demonstratesSecretusage by calling the Replicate API (replicate_api_token: Secret). The README recommends passing the token via an environment variable rather than a literal on the command line.Notes
cargo-denypyo3 advisory ignores (RUSTSEC-2026-0176/RUSTSEC-2026-0177) and thehello-replicatetest-harness wiring depend on Move examples from cog-examples to cog #3055. If this PR merges first, those may need to come along to keep CI green.