-
Notifications
You must be signed in to change notification settings - Fork 302
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
Support with_overrides setting metadata for map_task subnode instead of parent node #2982
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,6 +128,9 @@ def __init__( | |
**kwargs, | ||
) | ||
|
||
self.sub_node_metadata: NodeMetadata = super().construct_node_metadata() | ||
self.sub_node_metadata._name = self.name | ||
|
||
@property | ||
def name(self) -> str: | ||
return self._name | ||
|
@@ -137,15 +140,15 @@ def python_interface(self): | |
return self._collection_interface | ||
|
||
def construct_node_metadata(self) -> NodeMetadata: | ||
# TODO: add support for other Flyte entities | ||
""" | ||
This returns metadata for the parent ArrayNode, not the sub-node getting mapped over | ||
""" | ||
return NodeMetadata( | ||
name=self.name, | ||
) | ||
|
||
def construct_sub_node_metadata(self) -> NodeMetadata: | ||
nm = super().construct_node_metadata() | ||
nm._name = self.name | ||
return nm | ||
def get_sub_node_metadata(self) -> NodeMetadata: | ||
return self.sub_node_metadata | ||
Comment on lines
+150
to
+151
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The pattern in flytekit is to use a property + a private attribute: @property
def sub_node_metadata(self) -> NodeMetadata:
return self._sub_node_metadata |
||
|
||
@property | ||
def min_success_ratio(self) -> Optional[float]: | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -624,7 +624,7 @@ def get_serializable_array_node_map_task( | |||||
) | ||||||
node = workflow_model.Node( | ||||||
id=entity.name, | ||||||
metadata=entity.construct_sub_node_metadata(), | ||||||
metadata=entity.get_sub_node_metadata(), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider backward compatibility for method rename
Consider if renaming the method from Code suggestionCheck the AI-generated fix before applying
Suggested change
Code Review Run #a0a338 Is this a valid issue, or was it incorrectly flagged by the Agent?
|
||||||
inputs=node.bindings, | ||||||
upstream_node_ids=[], | ||||||
output_aliases=[], | ||||||
|
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.
Consider using the constructor parameters to set the
name
property when creatingNodeMetadata
instead of modifying the protected_name
attribute directly. This would follow better encapsulation practices.Code suggestion
Code Review Run #a0a338
Is this a valid issue, or was it incorrectly flagged by the Agent?