Modify arg default #512

PhilippeMoussalli · 2023-10-11T15:10:21Z

PR that replaces default data types to None

PhilippeMoussalli · 2023-10-11T15:16:12Z

components/load_from_hf_hub/src/main.py

@@ -57,6 +58,9 @@ def load(self) -> dd.DataFrame:
                    (subset_field_name, subset_field_name)
                columns.append(column_name)

+        if self.index_column is not None:


This is needed to keep the index column that will be dropped later on (if it exists). We weren't picking up on it since it was not in the component spec or remapping dict

RobbeSneyders

Thanks @PhilippeMoussalli!

RobbeSneyders · 2023-10-11T15:58:29Z

src/fondant/data_io.py

+                    f" to maximize worker usage",
+                )
+
+        elif self.input_partition_rows > 1:


Suggested change

elif self.input_partition_rows > 1:

elif self.input_partition_rows >= 1:

src/fondant/component_spec.py

RobbeSneyders · 2023-10-11T16:20:12Z

src/fondant/component_spec.py

@@ -47,7 +47,8 @@ def python_type(self) -> t.Any:
            "dict": json.loads,
            "list": json.loads,
        }
-        return lookup[self.type]
+        map_fn = lookup[self.type]
+        return lambda value: map_fn(value) if value != "None" else None  # type: ignore


Is this needed? I see that we only use this to register the arguments. Setting the type as None feels strange.

This can be a bit confusing so i'll recap a bit:

In kubeflow optional types that default to None are defined as optional in the spec with no constant runtime value. See input3 here and here. They also should not be passed to the componentOp so that's why we remove them here

In docker, all arguments are passed as strings. Above we're defining a map function above a type that converts back to their original type. In case that value is a None string, we're converting it back to a Nonetype. We actually has this in the v1 implementation, not sure why it was removed

Ok, I was a bit confused by the notation, but looking at it again, it's clear to me why this is needed. Thanks!

RobbeSneyders · 2023-10-11T16:22:24Z

components/load_from_hf_hub/fondant_component.yaml

@@ -19,11 +19,11 @@ args:
    description: Optional argument, a list containing the original image column names in case the 
      dataset on the hub contains them. Used to format the image from HF hub format to a byte string.
    type: list
-    default: []
+    default: None


I think for a list, [] makes more sense than None.

Also in the other components.

I am not sure I agree, then by definition it's either:

a normal argument with [] as default and no longer optional

Optional argument with [] as default which is not really common since Optional types don't really mix well with mutable data types

a normal argument with [] as default and no longer optional

If a default is defined, it is optional. It's just not KFP's isOptional. I don't see any issue with this.

Optional argument with [] as default which is not really common since Optional types don't really mix well with mutable data types

In Python this is an issue indeed, however we don't need to define the default in Python. We can just define the default in the fondant_component.yaml:

fondant_component.yaml

default: []

main.py

image_column_names: list,

We're just trying to indicate the absence of a value which is what None is better suited for. The empty list in this case won't be used for appending or modifying a certain behavior. Is there any added advantage compared to None?

It also seems like we're making an arbitrary choice on which data types to define as having empty values. Should we also include dictionaries?

The advantage is that you don't need to handle the None case in the code, and can always assume that the value is the of the type defined in the argument.

I would indeed include dictionaries as well. I don't think that's arbitrary.

This is btw just component implementation. Fondant supports None for list and dict as well. I just don't think we need to use it 😛

RobbeSneyders · 2023-10-12T13:20:49Z

src/fondant/component_spec.py

                arg_type_dict["isOptional"] = True
-            if arg.default is not None:
+            if arg.default is not None and arg.default != "None":


Do we need this? Shouldn't this PR make sure that "None" is no longer needed, but None can be used instead?

Good catch, it's not needed here indeed since this should be triggered at compilation

RobbeSneyders

Thanks @PhilippeMoussalli!

As mentioned, I would still change the defaults for collections in our own components to empty ones. But if we do or don't, the fondant functionality looks good to me!

PhilippeMoussalli · 2023-10-12T16:16:52Z

Thanks @PhilippeMoussalli!

As mentioned, I would still change the defaults for collections in our own components to empty ones. But if we do or don't, the fondant functionality looks good to me!

Thanks Robbe, I reverted the collections to their empty representations. So just to recap both arguments with defaults and arguments with default as None (IsOptional by kfp) are now marked at Optional in the python function without a defined data type. This makes it easy to have a single source of truth for the defaults which is in the component spec. I also reformatted both load_from_parquet and load_from_hub component since they were getting cluttered.

Could you have one last look please?

PhilippeMoussalli added 4 commits October 11, 2023 15:49

Modify way of passing optional args

1580075

Update tests and pipelines

962cafa

modify types to return None

8366062

modify input partiton rows

ce7d271

PhilippeMoussalli requested a review from RobbeSneyders October 11, 2023 15:10

PhilippeMoussalli linked an issue Oct 11, 2023 that may be closed by this pull request

Find a way to handle args with combined datatypes #437

Closed

PhilippeMoussalli added 2 commits October 11, 2023 17:11

Merge branch 'main' into modify-arg-default

53c5a75

add index to column to keep in load components

95eabef

PhilippeMoussalli commented Oct 11, 2023

View reviewed changes

add missing default value to input partition rows

4449db4

RobbeSneyders reviewed Oct 11, 2023

View reviewed changes

PhilippeMoussalli added 2 commits October 12, 2023 09:29

implement PR feedback

73071c8

add comment

ea6f05e

RobbeSneyders mentioned this pull request Oct 12, 2023

Bugfix input partition rows #515

Closed

RobbeSneyders reviewed Oct 12, 2023

View reviewed changes

PR feedback

8ba1196

RobbeSneyders approved these changes Oct 12, 2023

View reviewed changes

PhilippeMoussalli added 3 commits October 12, 2023 18:08

Change collections default from None to empty type

1408232

Update affected components and refactor them

e9df256

Merge branch 'main' into modify-arg-default

3c6fef4

RobbeSneyders approved these changes Oct 14, 2023

View reviewed changes

RobbeSneyders merged commit 15f875e into main Oct 14, 2023
4 checks passed

RobbeSneyders deleted the modify-arg-default branch October 14, 2023 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify arg default #512

Modify arg default #512

PhilippeMoussalli commented Oct 11, 2023

PhilippeMoussalli Oct 11, 2023

RobbeSneyders left a comment

RobbeSneyders Oct 11, 2023

RobbeSneyders Oct 11, 2023

PhilippeMoussalli Oct 12, 2023

RobbeSneyders Oct 12, 2023

RobbeSneyders Oct 11, 2023

PhilippeMoussalli Oct 12, 2023

RobbeSneyders Oct 12, 2023

PhilippeMoussalli Oct 12, 2023

RobbeSneyders Oct 12, 2023

RobbeSneyders Oct 12, 2023

PhilippeMoussalli Oct 12, 2023

RobbeSneyders left a comment

PhilippeMoussalli commented Oct 12, 2023

	elif self.input_partition_rows > 1:
	elif self.input_partition_rows >= 1:

Modify arg default #512

Modify arg default #512

Conversation

PhilippeMoussalli commented Oct 11, 2023

Choose a reason for hiding this comment

RobbeSneyders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobbeSneyders left a comment

Choose a reason for hiding this comment

PhilippeMoussalli commented Oct 12, 2023