-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(sdk): Allow keyword-only arguments in pipeline function signature #4544
fix(sdk): Allow keyword-only arguments in pipeline function signature #4544
Conversation
Hi @Udiknedormin. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
arg_type = None | ||
for input in pipeline_meta.inputs or []: | ||
if arg_name == input.name: | ||
arg_type = input.type | ||
break | ||
args_list.append(dsl.PipelineParam(sanitize_k8s_name(arg_name, True), param_type=arg_type)) | ||
param = dsl.PipelineParam(sanitize_k8s_name(arg_name, True), param_type=arg_type) | ||
if arg.kind == inspect.Parameter.KEYWORD_ONLY: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can put all arguments in a dictionary and then just use signature.bind which will sort out what goes where?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would work with position-only arguments:
def foo(a: str, /, b: str, *, c: str):
print(a, b, c)
import inspect
sig = inspect.signature(foo)
kargs = {"a": "A", "b": "B", "c": "C"}
bound_arguments = sig.bind(**kwargs) # Error: "a" is positional-only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Position-only arguments won't work with kwargs in signature.bind:
def foo(a: str, /, b: str, *, c: str): ...
import inspect
sig = inspect.signature(foo)
kwargs = {"a": "A", "b": "B", "c": "C"}
bound_arguments = sig.bind(**kwargs) # Error: "a" is positional-only
args = ["A"]
kwargs = {"b": "B", "c": "C"}
bound_arguments = sig.bind(**kwargs) # ok, no error
So it seems to me that the easiest approach is to only bound keyword-only arguments to kwargs, and the rest arguments to args.
/ok-to-test |
def test_keyword_only_argument_for_pipeline_func(self): | ||
def some_pipeline(casual_argument: str, *, keyword_only_argument: str): | ||
pass | ||
kfp.compiler.Compiler().compile(some_pipeline) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can pass None as path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right! I saw _compile
in tests but it was marked as "deprecated", so I just mindlessly replaced it, I guess... It should be _create_workflow
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_compile
is fine as well. We might de-deprecate it... The deprecation was due to a way TFX SDK was using the private compiler functions.
5405300
to
d04ad60
Compare
@Ark-kun I can see test failed with:
Looks like a dependency problem unrelated to this PR. |
/lgtm |
/approve |
/retest |
/hold |
pipeline_func_arg = basic.save_most_frequent_word | ||
|
||
# clone name and description | ||
@dsl.pipeline( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps @pipeline
is not needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should have the same name and description as save_most_frequent_word
, so it seems to me it does.
…s_in_pipeline_func
@Ark-kun I applied your suggestions. |
Thank you for your contribution and your patience. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun, Udiknedormin 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 |
/hold cancel |
@Udiknedormin: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
@Ark-kun Is there anything more that needs to be done before this PR is merged? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun, Udiknedormin 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 |
New changes are detected. LGTM label has been removed. |
I do not think so. I've approved this PR on Nov 21. Re-LGTMing after fixing the merge conflict /lgtm |
Support keyword-only arguments in pipeline function during compilation. Solves #4543, this is the suggested solution described there. Also adds test to check if the problem was solved
It seems it could be cherry-picked in the current release branch.