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
[Local] Support passing more parameters to handler if handler contains **kwargs
keyword
#3533
[Local] Support passing more parameters to handler if handler contains **kwargs
keyword
#3533
Conversation
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.
Looks good, does this also work with *args
?
Also, I get_func_arg
is also used with nuclio runtime, We should make sure it works there as well.
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.
There's some logic missing, I believe.
mlrun/runtimes/local.py
Outdated
kwargs[key] = copy(params[key]) | ||
for key in inputs: | ||
if key not in kwargs: | ||
kwargs[key] = context.get_input(key, inputs[key]) |
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.
The input processing above has more to it (using obj.local()
in some cases) - maybe have that piece of logic in an internal function, and use it here as well. Or did you verify this is dead code in which case I would remove it there as well?
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.
Straight to the point 💪
Just a minor suggestion.
mlrun/runtimes/local.py
Outdated
|
||
# VAR_KEYWORD meaning : A dict of keyword arguments that aren’t bound to any other parameter. | ||
# This corresponds to a **kwargs parameter in a Python function definition. | ||
if any(param.kind == param.VAR_KEYWORD for param in args.values()): |
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.
args = inspect.signature(handler).parameters
means that args
is an OrderedDict
and because **kwargs
can only be last in the function's parameters list, you can simply check the last one:
if any(param.kind == param.VAR_KEYWORD for param in args.values()): | |
if list(args.values())[-1].kind == param.VAR_KEYWORD: |
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.
minor
mlrun/runtimes/local.py
Outdated
else: | ||
return context.get_input(input_key, inputs[input_key]) |
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 believe it's the same
else: | |
return context.get_input(input_key, inputs[input_key]) | |
return input_obj |
tests/run/test_run.py
Outdated
@@ -142,6 +142,32 @@ def test_local_runtime_failure_before_executing_the_function_code(db): | |||
assert "failed on pre-loading" in str(exc.value) | |||
|
|||
|
|||
def test_local_runtime_with_kwargs(db): |
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.
would be nice if you could add some more test cases for example:
- if kwargs is empty
- if params is empty
- if param x is in kwargs
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.
Looks good.
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.
Minor
mlrun/runtimes/local.py
Outdated
kwargs[key] = _get_input_value(key) | ||
|
||
# get the last parameter, as **kwargs can only be last in the function's parameters list | ||
last_param = list(args.values())[-1] |
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.
What if list is empty ?
…s `**kwargs` keyword (mlrun#3533) (cherry picked from commit 00d1719)
https://jira.iguazeng.com/browse/ML-3761