-
Notifications
You must be signed in to change notification settings - Fork 300
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
Add node resource overrides #523
Conversation
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #523 +/- ##
=======================================
Coverage 85.37% 85.37%
=======================================
Files 369 369
Lines 28199 28276 +77
Branches 2281 2290 +9
=======================================
+ Hits 24074 24140 +66
- Misses 3505 3510 +5
- Partials 620 626 +6
Continue to review full report at Codecov.
|
flytekit/models/task.py
Outdated
@@ -100,8 +100,8 @@ def to_flyte_idl(self): | |||
:rtype: flyteidl.core.tasks_pb2.Resources | |||
""" | |||
return _core_task.Resources( | |||
requests=[r.to_flyte_idl() for r in self.requests], | |||
limits=[r.to_flyte_idl() for r in self.limits], | |||
requests=[r.to_flyte_idl() for r in self.requests] if self.requests is not None else 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.
we should revert this - locally at least, unit tests passed for me. #525
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.
but now that we can initialize the model with optionally empty requests or limits, we can't, right?
they fail for me
FAILED tests/flytekit/unit/models/core/test_workflow.py::test_workflow_template - TypeError: 'NoneType' object is not iterable
FAILED tests/flytekit/unit/models/core/test_workflow.py::test_task_node - TypeError: 'NoneType' object is not iterable
FAILED tests/flytekit/unit/models/core/test_workflow.py::test_node_task_with_no_inputs - TypeError: 'NoneType' object is not iterable
FAILED tests/flytekit/unit/models/core/test_workflow.py::test_node_task_with_inputs - TypeError: 'NoneType' object is not iterable
FAILED tests/flytekit/unit/models/core/test_workflow.py::test_branch_node - TypeError: 'NoneType' object is not iterable
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.
then why is this passing? #525
@@ -272,7 +273,9 @@ def get_serializable_node( | |||
inputs=entity.bindings, | |||
upstream_node_ids=[n.id for n in upstream_sdk_nodes], | |||
output_aliases=[], | |||
task_node=workflow_model.TaskNode(reference_id=task_spec.template.id), | |||
task_node=workflow_model.TaskNode( | |||
reference_id=task_spec.template.id, overrides=TaskNodeOverrides(resources=entity._resources) |
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.
These aren't the Resource models though right? They're the internal dataclass. Don't they need to be translated?
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.
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.
oh i see. can we update the type hint on line 36 of node.py then?
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.
ah yes, thanks for the catch
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
flytekit/models/task.py
Outdated
def __init__(self, requests, limits): | ||
def __init__( | ||
self, | ||
requests: typing.Optional[typing.List[ResourceEntry]] = 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.
nix this too?
Signed-off-by: Katrina Rogan <[email protected]>
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.
i meant just the optional part but cool, doesn't matter, already in the docstring anyways
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: Katrina Rogan [email protected]
TL;DR
Add node resource overrides
Type
Are all requirements met?
Complete description
How did you fix the bug, make the feature etc. Link to any design docs etc
Tracking Issue
flyteorg/flyte#475
flyteorg/flyte#1170
Follow-up issue
NA