Open
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive KFP (Kubeflow Pipelines) development and debugging demo called "pipe-fiction" that addresses challenges in developing and debugging ML pipelines. The demo provides multiple execution environments (subprocess, Docker, and cluster) with remote debugging capabilities.
Key changes:
- Demonstrates code organization patterns for KFP development with separation between core Python packages and pipeline orchestration
- Implements remote debugging infrastructure using debugpy for IDE integration across all execution environments
- Provides monkey patches for older KFP versions to enable port mapping and environment variable support in DockerRunner
Reviewed Changes
Copilot reviewed 20 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
kfp_docker_monkey_patches.py |
Monkey patches for KFP DockerRunner to enable port mapping and environment variables in older versions |
auth_session.py |
Utility for obtaining Istio/Dex authentication sessions for Kubeflow cluster access |
submit_to_cluster_from_remote.py |
Script for submitting pipelines to remote Kubeflow clusters with authentication |
submit_to_cluster_from_kf_notebook.py |
Simple pipeline submission script for use within Kubeflow notebooks |
run_locally_in_subproc.py |
Local pipeline execution using subprocess runner |
run_locally_in_docker.py |
Local pipeline execution using Docker runner with debugging support |
pipeline.py |
Main pipeline definition with debugging configuration |
components.py |
KFP component definitions with remote debugging capabilities |
| Various config files | Project configuration, dependencies, and VS Code debugging setup |
Comments suppressed due to low confidence (1)
pipelines/pipe-fiction/pipelines/pyproject.toml:13
- pip version 25.1.1 does not exist. As of my knowledge cutoff in January 2025, the latest pip version was in the 24.x series. This version specification will cause dependency resolution to fail.
"pip>=25.1.1",
pipelines/pipe-fiction/pipe-fiction-codebase/pipe_fiction/data_generator.py
Outdated
Show resolved
Hide resolved
pipelines/pipe-fiction/pipelines/utils/kfp_docker_monkey_patches.py
Outdated
Show resolved
Hide resolved
…_generator.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…es.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
tmvfb
previously requested changes
Oct 7, 2025
Collaborator
|
@hsteude, in general awesome job, I had no idea about KFP debugging and I also never used VSCode, so I had to download and take a look, worked for me, and was well-written and understandable. My main feedback is as follows:
|
- Replace Dex-based auth_session.py with Keycloak auth using temp OIDC client pattern (create client -> get user token -> cleanup) and KFP existing_token - Update submit_to_cluster_from_remote.py to use token-based auth - Fix all stale script references in README (run_in_k8s_cluster.py, submit_to_cluster.py -> actual filenames) - Restructure Remote Debugging section into 4 clear subsections - Add cluster submission docs with env var examples - Sync README launch.json and directory listing with actual files - Fix .gitignore typo (*.py[codz] -> *.py[cod]) - Fix kfp_docker_monkey_patches.py docstring import path - Fix debug parameter name in Debugging Architecture section - Add DAP compatibility note for non-VS Code IDEs
… flow Remove setup_keycloak_client.py and all admin/temp-client machinery from auth_session.py. The module now contains only get_user_token() which uses a pre-existing OIDC client created by the admin via the Keycloak UI. This removes the need for Keycloak admin credentials at submission time and simplifies the codebase for this PR.
Collaborator
|
@hsteude, I addressed all the feedback that was there and integrated that properly into our setup with relevant readme updates. However, I can't review anymore, so we could either discuss and test together, or add someone else to the discussion |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 25 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pipelines/pipe-fiction/pipelines/utils/kfp_docker_monkey_patches.py
Outdated
Show resolved
Hide resolved
- Fix type annotations: List -> List[str], Dict[str,str] -> Dict[str,object], any -> Any
- Make TLS verification configurable via verify_ssl param and VERIFY_SSL env var
- Fix image.split(':') -> rsplit(':', 1) for registry:port image refs
- Add allow-list filtering for extra_args in DockerRunner monkey patch
- Call original DockerRunner.__init__ before applying patches
- Validate debug parameter exists in decorated function signature
- Fix raw-string docstring delimiter extraction (r-prefix handling)
- Normalize packages_to_install to new list to prevent mutation side effects
- Replace print() with logger calls for import-time messages
- Fix docstring import example to match actual module path
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The readme file that live at
pielines/pipe-fiction/README.mdwill hopefully explain why this might be useful and how to test it.