Skip to content
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

Lists are handled incorrectly by python components #1901

Closed
gabrielvcbessa opened this issue Aug 20, 2019 · 7 comments · Fixed by #1945
Closed

Lists are handled incorrectly by python components #1901

gabrielvcbessa opened this issue Aug 20, 2019 · 7 comments · Fixed by #1945

Comments

@gabrielvcbessa
Copy link

gabrielvcbessa commented Aug 20, 2019

Take as example a function max_value that will take as an input lists of ints and will return the biggest value.

If we pass as an input one hardcoded list, in execution time it would receive it as an string. Something like this:

"[1, 32]"

If we test it using some output from previous methods the input would be something like this:

"[{'PipelineParam': {'name': 'output', 'op_name': 'Return n', 'value': None, 'param_type': <kfp.dsl._metadata.TypeMeta object at 0x10dcb5748>, 'pattern': '1'}}, {'PipelineParam': {'name': 'output', 'op_name': 'Return n 2', 'value': None, 'param_type': <kfp.dsl._metadata.TypeMeta object at 0x10dcb5748>, 'pattern': '32'}}]"
from typing import List
from kfp.components import func_to_container_op


def return_n(n: int) -> int:
    print(n)
    return n


def max_value(values: List[int]) -> int:
    print(values)
    return max(*values)


return_n_op = func_to_container_op(return_n, extra_code='from typing import List')
max_value_op = func_to_container_op(max_value, extra_code='from typing import List')


def pipeline():
    max_value_op([1, 32])  # The max value here will be "]"

    first_value = return_n_op(1).output
    second_value = return_n_op(32).output
    max_value_op([first_value, second_value])  # The max value here will be "}"
@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 22, 2019

Thanks for the issue. Issues from users help us prioritize the features.

KF Pipelines orchestrates containerized command-line programs.
Command-line programs, on the lowest level, only pass strings or blobs/files.
All other types need to be explicitly supported (by providing serialization/deserialization routines).

At this moment only str, int, float are supported.
Good news is that I've written support for List and Dict and it will be available soon.
I'm also improving the value checking, so in future you'll get warnings about converting values of unsupported types to strings.

@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 22, 2019

Hmm. The generic lists probably won't be supported in the first release. I'll think of how to support them better.

@pavan-infiswift
Copy link

Thanks for the clarification @Ark-kun

@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 22, 2019

There is another issue in your code that would remain unfixed for some time:
The components are supposed to have strict signature with a fixed number of inputs and outputs. You're trying to have variable-length inputs by passing the outputs from variable number of upstream tasks.

max_value_op([str(first_value), str(second_value)])

might work, but it's not supported. And in this case (with the feature support I'll add soon) you'll get a list of strings, not numbers.

@saltazaur
Copy link

HI,

@Ark-kun have you added support forlist?
It seems, that is doesn't work in Kubeflow version: 1.0.4.

@kolakows
Copy link

Works Kubeflow 1.2.

@saltazaur, at first I tried foo: List[str], only after I realized that's yet unsupported generic list. foo: list correctly passed items in list.

@esantix
Copy link

esantix commented Nov 4, 2022

for arguments parsing in Kubeflow I recommend json.loads

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this issue Oct 22, 2023
* Add preliminary ServingRuntime support

* Add test and some refactoring

* Added ReadinessProbe to ServingRuntime Container spec

* More refactoring

* Add more tests

* address comments

* Add check for MMS compatible runtime

* Also add a few more tests

* Remove runtimes from main installation

* Update error messages

* Fix index out of bounds bug

* Update some docstrings and add more unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants