Repository navigation
Conversation
Contributor Report for @jeffspahr
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is consistent across SDK and CLI surfaces and includes comprehensive regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Adds explicit namespace selection for pipeline-by-name SDK and CLI operations while preserving shared-pipeline defaults.
Changes:
- Adds namespace-aware pipeline version uploads and CLI lookups.
- Improves missing-parent errors and rejects incompatible selectors.
- Adds coverage for scoped lookup, failures, permissions, and ID behavior.
🤖 LGTM 🤖
| File | Description |
|---|---|
sdk/python/kfp/client/client.py |
Adds namespace-aware version upload APIs. |
sdk/python/kfp/client/client_test.py |
Tests SDK lookup and validation behavior. |
sdk/python/kfp/cli/run.py |
Adds namespace selection to run creation. |
sdk/python/kfp/cli/pipeline.py |
Adds namespace selection to version creation. |
sdk/python/kfp/cli/cli_test.py |
Tests CLI scope and failure behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
b0862dc to
8803fe7
Compare
8803fe7 to
06d6380
Compare
AP agent review — #14712: fix(sdk): support explicit pipeline namespaces in by-name callersReviewed head: Classification: Two independent read-only agent reviewers assessed this head. Tests were inspected but not executed; this does not certify CI or live deployment behavior. Correctness / RiskAdds an optional pipeline-namespace selector to the SDK upload_pipeline_version helpers and to the CLI create-version and run create commands, and fails early with an actionable error when a by-name lookup finds nothing. The control flow is correct: omitted namespace still searches shared pipelines only, the client/global default namespace is never inherited, namespace with an explicit ID is rejected before any API call, and the ID-only path is byte-for-byte unchanged. The early failure in run create is an improvement over the base, where a failed name lookup with --version created the experiment and then hit a ValueError in _create_job_config. Test mocks match the real call signatures and the CI environment (requires-python >=3.11, Click 8.3.1) supports the enterContext and result.output usage. Only non-blocking notes remain. Standards / TestsAdds an optional |
|
AA review summary for #14712: fix(sdk): support explicit pipeline namespaces in by-name callers
|
|
@droctothorpe Addressed the optional coverage/documentation notes in #14712 (comment) in fbe674f. Added CLI rejection tests for I kept the short error messages local to each CLI command: sharing them would add cross-command coupling solely for text reuse without changing the contract. 205 targeted CLI/client tests plus 8 subtests and applicable hooks pass. Hosted CI must validate the new head. |
|
AA review summary for #14712: fix(sdk): support explicit pipeline namespaces in by-name callers
|
fbe674f to
d10a2f0
Compare
b9c1edb to
5e07197
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
AA review summary for #14712: fix(sdk): support explicit pipeline namespaces in by-name callers
|
5e07197 to
fee4e86
Compare
Signed-off-by: Jeff Spahr <spahrj@gmail.com>
Signed-off-by: Jeff Spahr <spahrj@gmail.com>
Signed-off-by: Jeff Spahr <spahrj@gmail.com>
Signed-off-by: Jeff Spahr <spahrj@gmail.com>
fee4e86 to
5363605
Compare
Signed-off-by: Jeff Spahr <spahrj@gmail.com>
Problem
By-name SDK and CLI calls do not expose the pipeline namespace even though explicit namespace lookup already exists. Private pipelines can be missed or a same-named shared pipeline selected, forcing callers to resolve IDs manually.
Summary
Make SDK/CLI by-name operations usable for private pipelines without requiring a separate lookup followed by an ID-based call. This addresses an existing client usability gap, not a newly introduced security regression.
Motivation and why now
The SDK already supports
get_pipeline_id(name, namespace=...), but the version-upload helpers and CLI run/version commands call it without exposing a pipeline-namespace argument. Those operations therefore search shared pipelines, even when the client or CLI has a default experiment namespace. A private pipeline may not be found; a same-named shared pipeline may be selected instead. Users can already select a private pipeline by resolving its ID with an explicit namespace and then using the ID-based operation. This PR makes that existing workflow available directly in the convenience methods and commands.Reviewing the namespace-lookup security fixes (#14052 and #14522) and 2.18 upgrade guidance surfaced this gap. These SDK/CLI paths use filtered
ListPipelines, not the affectedGetPipelineByNameendpoint, and did not rely on its unsafe defaults. This PR is not needed to preserve ordinary SDK/CLI compatibility with those backend fixes and is not an additional authorization fix.Changes
namespace=to SDKupload_pipeline_versionand itsfrom_pipeline_funcwrapper.--pipeline-namespaceto CLIpipeline create-versionandrun createwhen resolving--pipeline-name.For example:
The global CLI
--namespacecontinues to scope the experiment; it does not select the pipeline lookup scope. A private pipeline/version still needs an experiment in its owning namespace and normal permissions.Compatibility and release tracking
Related to the caller review in #14421 and documentation in #14703. Target master; any release-2.18 backport is a separate usability decision tracked in #14409, not a prerequisite for the namespace security fixes or a security release gate. Older clients can continue using the existing explicitly scoped lookup followed by an ID-based call. Installing a backend release does not update SDK/CLI installations.
The missing-parent behavior is deliberately earlier and clearer: the old SDK proceeded to upload with
pipelineid=None; the new SDK raisesValueErrorbefore making that request.get_pipeline_iditself retains its Optional return contract. No automatic namespace fallback, authorization relaxation, or cluster configuration change is introduced.Verification
Checklist
Review follow-up:
fbe674f10adds invalid namespace-selector CLI coverage and SDK release guidance, and wraps long expressions. 205 targeted CLI/client tests plus 8 subtests and applicable hooks pass; hosted CI must validate the new head. The prior full-suite results above remain scoped to the preceding revision.CI prerequisite
Includes the focused runner compatibility fix from #14845 after a confirmed missing-libpython failure before SDK client tests. The #14845 prerequisite has merged; the namespace implementation is unchanged.
Current validation
After the current-base refresh, 206 client/CLI tests and eight subtests pass; eight Docker mirror regression tests also pass. Hosted CI is running on the updated branch; earlier green runs do not establish current-head completion.
Shared MLflow CI prerequisite
The compatible immutable MLflow runtime/operator pins are now inherited from merged #14861. The independent fixture pin regression is retained. Database migration and serving assertions remain intact; no migration or replica-readiness bypass is introduced.
Documentation CI prerequisite
Includes #14863 to build SDK documentation with system fonts and no Google Fonts downloads. The no-network regression builds both actual font configurations; documentation content checks remain intact.
Includes the GitHub CLI provisioning prerequisite consolidated into #14845 for managed runners. Seven setup regressions and twelve artifact-polling tests pass. Shared #14845 runner compatibility fixes are retained; these prerequisites do not change product behavior.
Conflict-resolution validation
Rebased onto master
2ca202210. The resulting tree exactly matches an independently resolved merge, preserving the product implementation. CI guidance retains upstream Istio instructions and the applicable PR guidance. Whitespace checks passed. Eight Docker mirror tests and seven GitHub CLI provisioning tests passed for the inherited runner setup. Earlier application-test results above describe the preceding revisions; fresh hosted CI is pending forfee4e865a.E2E runner contention
The TLS E2E lane timed out while ten Ginkgo workers shared a four-CPU runner with Kind and pipeline pods: CPU averaged 97%, load averaged 43.3 (peak 60.1), and CPU pressure stalled work for 91.4% of the test window. Automatic single-user E2E concurrency is now capped at half the available CPUs (minimum one), retaining any stricter lane cap. Explicit workflow-dispatch overrides remain authoritative. This changes neither selected specs nor suite timeouts.
The configure-shell regression fails against the previous workflow and passes with the cap, including one/four/32-CPU runners, stricter failure/nested lane caps, label preservation, and an explicit seven-worker override. Both concurrency tests and five existing API shard tests pass; Ruff and whitespace validation pass. Hosted validation on
7d40b8a67passes every substantive check. The previously timed-out TLS lane passed all 33 selected specs in 1,407 seconds (~23.5 minutes), with unchanged labels and timeout. On its four-CPU runner, CPU averaged 15%, load averaged 1.6 (peak 3.6), and CPU-pressure stalls fell to 5.42% of the test window.