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

update array node model #3156

Merged
merged 4 commits into from
Feb 27, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions flytekit/core/array_node.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,10 @@ def execution_mode(self) -> _core_workflow.ArrayNode.ExecutionMode:
def is_original_sub_node_interface(self) -> bool:
return True

@property
def bound_inputs(self) -> Set[str]:
return set()
Comment on lines +232 to +233
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider returning instance variable for bound_inputs

The bound_inputs property appears to be returning an empty set while there is an instance variable self._bound_inputs that seems to be the intended return value. Consider returning self._bound_inputs instead of an empty set.

Code suggestion
Check the AI-generated fix before applying
Suggested change
def bound_inputs(self) -> Set[str]:
return set()
def bound_inputs(self) -> Set[str]:
return self._bound_inputs

Code Review Run #73d43e


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them


def __call__(self, *args, **kwargs):
if not self._bindings:
ctx = FlyteContext.current_context()
Expand Down
3 changes: 3 additions & 0 deletions flytekit/models/core/workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ def __init__(
execution_mode=None,
is_original_sub_node_interface=False,
data_mode=None,
bound_inputs=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding type hints for bound_inputs

Consider adding type hints for the bound_inputs parameter in the constructor. This would help with code maintainability and IDE support.

Code suggestion
Check the AI-generated fix before applying
 -        node: "Node",
 -        bound_inputs=None,
 +        node: "Node",
 +        bound_inputs: typing.Optional[typing.List[_Binding]] = None,

Code Review Run #73d43e


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

) -> None:
"""
TODO: docstring
Expand All @@ -407,6 +408,7 @@ def __init__(
self._execution_mode = execution_mode
self._is_original_sub_node_interface = is_original_sub_node_interface
self._data_mode = data_mode
self._bound_inputs = bound_inputs

@property
def node(self) -> "Node":
Expand All @@ -421,6 +423,7 @@ def to_flyte_idl(self) -> _core_workflow.ArrayNode:
execution_mode=self._execution_mode,
is_original_sub_node_interface=BoolValue(value=self._is_original_sub_node_interface),
data_mode=self._data_mode,
bound_inputs=self._bound_inputs,
)

@classmethod
Expand Down
1 change: 1 addition & 0 deletions flytekit/tools/translator.py
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,7 @@ def get_serializable_array_node(
execution_mode=array_node.execution_mode,
is_original_sub_node_interface=array_node.is_original_sub_node_interface,
data_mode=array_node.data_mode,
bound_inputs=array_node.bound_inputs,
)


Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ dependencies = [
"diskcache>=5.2.1",
"docker>=4.0.0",
"docstring-parser>=0.9.0",
"flyteidl>=1.15.0",
"flyteidl>=1.15.1",
"fsspec>=2023.3.0",
"gcsfs>=2023.3.0",
"googleapis-common-protos>=1.57",
Expand Down
Loading