-
Notifications
You must be signed in to change notification settings - Fork 305
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
Override node name #1344
Override node name #1344
Conversation
Signed-off-by: Hongxin Liang <[email protected]>
Signed-off-by: Hongxin Liang <[email protected]>
Signed-off-by: Hongxin Liang <[email protected]>
@@ -401,3 +397,24 @@ def my_wf(a: str): | |||
_resources_models.ResourceEntry(_resources_models.ResourceName.CPU, "1"), | |||
_resources_models.ResourceEntry(_resources_models.ResourceName.MEMORY, "100"), | |||
] | |||
|
|||
|
|||
def test_name_override(): |
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.
Real change here.
@@ -87,6 +87,7 @@ def empty_wf2(): | |||
wf_spec = get_serializable(OrderedDict(), serialization_settings, empty_wf2) | |||
assert wf_spec.template.nodes[0].upstream_node_ids[0] == "n1" | |||
assert wf_spec.template.nodes[0].id == "n0" | |||
assert wf_spec.template.nodes[0].metadata.name == "t2" |
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.
Real change here.
|
It's not related to your change. I'm actively investigating this. |
@eapolinario Thanks for heads-up! |
Signed-off-by: Yee Hing Tong <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1344 +/- ##
==========================================
- Coverage 68.85% 68.79% -0.07%
==========================================
Files 287 287
Lines 26453 26462 +9
Branches 2497 2498 +1
==========================================
- Hits 18215 18204 -11
- Misses 7750 7773 +23
+ Partials 488 485 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Hmm I guess I was using a different black version. I will try to fix that. |
Signed-off-by: Yee Hing Tong <[email protected]>
@honnix i just ran make fmt |
65a5ef0
to
dfd3487
Compare
Sorry I missed the message. I reverted my commits. |
Thanks. The pre-commit thing. Nice! |
Signed-off-by: Hongxin Liang [email protected]
TL;DR
Override node name.
Type
Are all requirements met?
Complete description
More details are captured in #3092.
I tested this on our setup. Everything seems to work fine, except the
Nodes
tab of the execution detail view. WhileGraph
andTimeline
both show the overridden node name,Nodes
tab still shows the original task name.Also note that many changes were made by
black
.Tracking Issue
flyteorg/flyte#3092
Follow-up issue
NA