feat(sdk): add runtime logic for custom artifact types (support for custom artifact types pt. 3) #8233
Conversation
|
Skipping CI for Draft Pull Request. |
|
/test all |
connor-mccarthy
changed the title
connor-mccarthy
force-pushed
the
add-custom-artifact-type-runtime-logic
branch
from
189a579 to
d74f0cf
Compare
connor-mccarthy
changed the title
connor-mccarthy
force-pushed
the
add-custom-artifact-type-runtime-logic
branch
from
d74f0cf to
2dd1d94
Compare
|
/retest |
|
/retest |
connor-mccarthy
changed the title
| for arg in args: | ||
| annotation = arg.annotation | ||
| # Handle InputPath() and OutputPath() | ||
| if isinstance(annotation, ast.Call): |
There was a problem hiding this comment.
Don't quite follow the code, would it falls into this condition if users mistakenly use Input(...), and what if it's some other unexpected calls like PipelineParam(...)?
There was a problem hiding this comment.
Discussed offline and removed handling for InputPath and OutputPath.
I agree that we should program more defensively around unexpected inputs, though I think this belongs in the kfp.components.extract_component_interface function. The issue of liberal acceptance of annotations is an issue that precedes this PR. WDYT about this as a separate work item?
There was a problem hiding this comment.
though I think this belongs in the
kfp.components.extract_component_interfacefunction
I think you're probably right. I recall we do check and make sure the type annotation is one of our known usage, otherwise there will be compilation errors. As long as this change doesn't introduce regression to this UX, which I now think it probably doesn't, we should be all good.
|
/retest |
|
@connor-mccarthy: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
With fcd56b3 I implemented some significant refactoring which makes the code not only cleaner, but also safer by not going down the ast parsing code path unless required. Before this commit, the code used ast parsing for every lightweight component function. |
| "third_message": "World" | ||
| } | ||
| }, | ||
| "outputs": { | ||
| "parameters": { | ||
| "output": { | ||
| "Output": { |
There was a problem hiding this comment.
Curious why do we have this diff? I guess executor input json is case-insensitive, you made the change so that it's more "correct" as KFP use Output as single return output name?
There was a problem hiding this comment.
That's exactly it: when a component of this type is executed this is what the executor input looks like.
| @@ -483,59 +615,63 @@ def test_artifact_output(self): | |||
| } | |||
| """ | |||
|
|
|||
| def test_func(first: str, second: str) -> Artifact: | |||
| def test_func(first: str, second: str, output: Output[Artifact]) -> str: | |||
There was a problem hiding this comment.
Keep a test case with -> Artifact? (likely don't need to change this test func at all)
| 'artifact_class_base_symbol': 'c', | ||
| 'qualname': 'a.b.c.d.e', | ||
| 'expected': 'a.b.c' | ||
| }, |
There was a problem hiding this comment.
For my education, what would be a real-world case for the last case?
There was a problem hiding this comment.
The case is where a user annotates with an artifact defined deeply in a package, but only imports a high-level module:
For example:
from google.cloud import aiplatform
@dsl.component
def comp(vertex_model: Output[aiplatform.metadata.schema.google.artifact_schema.VertexModel]):
....
|
/unhold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ustom artifact types pt. 3) (kubeflow#8233) * add runtime artifact instance creation logic * refactor executor * add executor tests * add custom artifact type import handling and tests * fix artifact class construction * fix custom artifact type in tests * add typing extensions dependency for all python versions * use mock google namespace artifact for tests * remove print statement * update google artifact golden snapshot * resolve some review feedback * remove handling for OutputPath and InputPath custom artifact types; update function names and tests * clarify named tuple tests * update executor tests * add artifact return and named tuple support; refactor; clean tests * implement review feedback; clean up artifact names * move test method
Description of your changes:
Adds runtime handling to support use of custom artifact types from non-kfp libraries in lightweight components. This includes:
ephemeral_component.pyat runtimeChecklist:
convention used in this repository.