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

Enable optional component arguments #201

Merged
merged 19 commits into from
Jun 15, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion fondant/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ def _add_and_parse_args(cls, spec: ComponentSpec):
component_arguments = cls._get_component_arguments(spec)

for arg in component_arguments.values():
# Input manifest is not required for loading component
if arg.name in cls.optional_fondant_arguments():
input_required = False
default = None
Expand Down
14 changes: 7 additions & 7 deletions fondant/component_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@

# TODO: remove after upgrading to kfpv2
kubeflow2python_type = {
"String": str,
"Integer": int,
"Float": float,
"Boolean": ast.literal_eval,
"JsonObject": json.loads,
"JsonArray": json.loads,
"String": lambda value: str(value) if value != "None" else None,
"Integer": lambda value: int(value) if value != "None" else None,
"Float": lambda value: float(value) if value != "None" else None,
"Boolean": lambda value: ast.literal_eval(value) if value != "None" else None,
"JsonObject": lambda value: json.loads(value) if value != "None" else None,
"JsonArray": lambda value: json.loads(value) if value != "None" else None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense converting this into a function.

Suggested change
"String": lambda value: str(value) if value != "None" else None,
"Integer": lambda value: int(value) if value != "None" else None,
"Float": lambda value: float(value) if value != "None" else None,
"Boolean": lambda value: ast.literal_eval(value) if value != "None" else None,
"JsonObject": lambda value: json.loads(value) if value != "None" else None,
"JsonArray": lambda value: json.loads(value) if value != "None" else None,
kubeflow_to_python_type_dict = {
"String": str,
"Integer": int,
"Float": float,
"Boolean": ast.literal_eval,
"JsonObject": json.loads,
"JsonArray": json.loads,
}
def kubeflow_to_python_type(type_: str) -> t.Any:
map_fn = kubeflow_to_python_type_dict[type_]
return lambda value: map_fn(value) if value != "None" else None

Copy link
Collaborator

@GeorgesLorre GeorgesLorre Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or use the match case syntax: https://peps.python.org/pep-0622/

def kubeflow_to_python_type(type: str):
    match type:
        case "String": lambda value: str(value) if value != "None" else None
        case "Integer": lambda value: int(value) if value != "None" else None
        case "Float": lambda value: float(value) if value != "None" else None
        case "Boolean": lambda value: ast.literal_eval(value) if value != "None" else None
        case "JsonObject": lambda value: json.loads(value) if value != "None" else None
        case "JsonArray": lambda value: json.loads(value) if value != "None" else None
        case _: raise ValueError(f"Unknown type: {type}")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore the above match case is python 3.10+ and fondant is 3.8+

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback :) I think it's ready to go

}
# TODO: Change after upgrading to kfp v2
# :https://www.kubeflow.org/docs/components/pipelines/v2/data-types/parameters/
Expand All @@ -47,7 +47,6 @@ class Argument:
description: argument description
type: the python argument type (str, int, ...)
default: default value of the argument (defaults to None)

"""

name: str
Expand Down Expand Up @@ -234,6 +233,7 @@ def from_fondant_component_spec(
"name": "component_spec",
"description": "The component specification as a dictionary",
"type": "JsonObject",
"default": "None",
},
*(
{
Expand Down
2 changes: 1 addition & 1 deletion fondant/schemas/component_spec.json
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,4 @@
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ inputs:
- name: component_spec
description: The component specification as a dictionary
type: JsonObject
default: None
- name: storage_args
description: Storage arguments
type: String
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,14 @@ args:
description: default float argument
type: float
default: 3.14
bool_default_arg:
bool_false_default_arg:
description: default bool argument
type: bool
default: 'False'
bool_true_default_arg:
description: default bool argument
type: bool
default: 'True'
list_default_arg:
description: default list argument
type: list
Expand All @@ -27,7 +31,39 @@ args:
description: default dict argument
type: dict
default: '{"foo":1, "bar":2}'
override_default_string_arg:
description: default argument that can be overriden
string_default_arg_none:
description: default string argument
type: str
default: None
integer_default_arg_none:
description: default integer argument
type: int
default: None
float_default_arg_none:
description: default float argument
type: float
default: None
bool_default_arg_none:
description: default bool argument
type: bool
default: None
list_default_arg_none:
description: default list argument
type: list
default: None
dict_default_arg_none:
description: default dict argument
type: dict
default: None
override_default_arg:
description: argument with default python value type that can be overriden
type: str
default: foo
override_default_none_arg:
description: argument with default None value type that can be overriden with a valid python type
type: float
default: None
override_default_arg_with_none:
description: argument with default python type that can be overriden with None
type: str

38 changes: 30 additions & 8 deletions tests/test_component.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,9 @@ def write(self, dataframe, *, flag, value):
MyComponent.from_args()


def test_default_args_component(tmp_path_factory, monkeypatch):
"""Test that default arguments defined in the fondant spec are passed correctly and have the
proper data type.
def test_default_and_optional_args_component(tmp_path_factory, monkeypatch):
"""Test that default arguments and optional defined in the fondant spec are passed correctly
and have the proper data type and assigned value.
"""

# Mock `Dataset.load_dataframe` so no actual data is loaded
Expand All @@ -219,20 +219,38 @@ def write(
string_default_arg,
integer_default_arg,
float_default_arg,
bool_default_arg,
bool_true_default_arg,
bool_false_default_arg,
list_default_arg,
dict_default_arg,
override_default_string_arg,
string_default_arg_none,
integer_default_arg_none,
float_default_arg_none,
bool_default_arg_none,
list_default_arg_none,
dict_default_arg_none,
override_default_arg,
override_default_none_arg,
override_default_arg_with_none,
):
float_const = 3.14
# Mock write function that sinks final data to a local directory
assert string_default_arg == "foo"
assert integer_default_arg == 1
assert float_default_arg == float_const
assert bool_default_arg is False
assert bool_false_default_arg is False
assert bool_true_default_arg is True
assert list_default_arg == ["foo", "bar"]
assert dict_default_arg == {"foo": 1, "bar": 2}
assert override_default_string_arg == "bar"
assert string_default_arg_none is None
assert integer_default_arg_none is None
assert float_default_arg_none is None
assert bool_default_arg_none is None
assert list_default_arg_none is None
assert dict_default_arg_none is None
assert override_default_arg == "bar"
assert override_default_none_arg == float_const
assert override_default_arg_with_none is None

# Mock CLI arguments
sys.argv = [
Expand All @@ -245,8 +263,12 @@ def write(
"",
"--component_spec",
f"{component_spec_string}",
"--override_default_string_arg",
"--override_default_arg",
"bar",
"--override_default_none_arg",
"3.14",
"--override_default_arg_with_none",
"None",
]

# # Instantiate and run component
Expand Down