-
Notifications
You must be signed in to change notification settings - Fork 25
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
Modify arg default #524
Modify arg default #524
Conversation
src/fondant/component_spec.py
Outdated
@@ -191,14 +192,22 @@ def outputs_additional_subsets(self) -> bool: | |||
|
|||
@property | |||
def args(self) -> t.Mapping[str, Argument]: | |||
def _get_default(argument_info): | |||
if "default" in argument_info: | |||
if argument_info["default"] == "None": |
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.
This is needed for arguments that are taken from the componentSpec which always have a string type
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 might be a bit cleaner to include this in the Argument
class. I think this should work:
@dataclass
class Argument:
default: t.Any
def __post_init__(self):
self.default = None if self.default == "None" else self.default
@@ -289,6 +290,30 @@ def compile( | |||
pipeline.validate(run_id=run_id) | |||
logger.info(f"Compiling {pipeline.name} to {output_path}") | |||
|
|||
def set_component_exec_args( |
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.
This was previously defined in the ComponentSpec but in fact should be evaluated at runtime. Optional arguments with defaults should not be included in the arg section of the componentOp otherwise some errors occur
I1016 10:42:19.355692 27 cache.go:116] Connecting to cache endpoint 10.0.12.128:8887
[2023-10-16 10:42:21,330 | fondant.cli | INFO] Component `LoadFromHubComponent` found in module main
usage: fondant [-h] [--component_spec COMPONENT_SPEC] [--cache CACHE]
[--input_partition_rows INPUT_PARTITION_ROWS]
[--cluster_type CLUSTER_TYPE] [--client_kwargs CLIENT_KWARGS]
fondant: error: argument --input_partition_rows: invalid int value: "{{$.inputs.parameters['input_partition_rows']}}"
c4df0b4
to
b37a727
Compare
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.
Thanks @PhilippeMoussalli! The pipeline that was failing before now succeeds.
src/fondant/component_spec.py
Outdated
@@ -191,14 +192,22 @@ def outputs_additional_subsets(self) -> bool: | |||
|
|||
@property | |||
def args(self) -> t.Mapping[str, Argument]: | |||
def _get_default(argument_info): | |||
if "default" in argument_info: | |||
if argument_info["default"] == "None": |
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 might be a bit cleaner to include this in the Argument
class. I think this should work:
@dataclass
class Argument:
default: t.Any
def __post_init__(self):
self.default = None if self.default == "None" else self.default
src/fondant/component_spec.py
Outdated
@@ -34,7 +34,7 @@ class Argument: | |||
name: str | |||
description: str | |||
type: str | |||
default: t.Any = None | |||
default: t.Optional[t.Any] = None |
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.
t.Any
already includes t.Optional
.
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.
ahh good to know
Glad it works :) I hope everything is fine now with the argument but some things might still pop up |
No description provided.