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

Upstream/node event update #5528

Merged
merged 2 commits into from
Jul 2, 2024
Merged

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Jul 1, 2024

Why are the changes needed?

This PR upstreams most of a change from union (see "Add fields to NodeExecutionEvent").

What changes were proposed in this pull request?

From upstream,

The solution proposed here is to simply have propeller add the subworkflow ID, since propeller has this information already readily accessible in the node execution context.

Propeller also adds a boolean to the NodeExecutionEvent indicating whether or not this node corresponds to an entity that was at some point, generated by a dynamic task. Once a dynamic task runs, it comes with its own set of task and subworkflow definitions. This boolean just indicates to the events handler to not try to look up task/workflow template information in the Admin db as they may not even have been registered.

Changes

  • Add fields target_entity and is_in_dynamic_chain to the NodeExecutionEvent message. The target_entity field is currently only filled in for subworkflows.
    *Add IsInDynamicChain to the ImmutableParentInfo interface.
  • Update ToNodeExecutionEvent to take a sub workflow ID, and pull out the dynamic chain boolean information.
  • Update sendEvents in array node handling to read and send the dynamic chain boolean.
  • Add a test for golang pointer gotcha I'm always forgetting.

However, the part that actually adds the subworkflow id is not available, as it relies on another change that will not be upstreamed (ExecutableSubWorkflow interface doesn’t have GetIdentifier)

How was this patch tested?

Tested in union.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 59.75610% with 33 lines in your changes missing coverage. Please review.

Project coverage is 60.97%. Comparing base (5b0d787) to head (f6f5f95).
Report is 116 commits behind head on master.

Files with missing lines Patch % Lines
...lytepropeller/pkg/controller/nodes/common/utils.go 16.66% 15 Missing ⚠️
flytepropeller/pkg/controller/nodes/executor.go 55.55% 7 Missing and 1 partial ⚠️
...ler/pkg/apis/flyteworkflow/v1alpha1/subworkflow.go 0.00% 6 Missing ⚠️
...propeller/pkg/apis/flyteworkflow/v1alpha1/nodes.go 60.00% 2 Missing ⚠️
...eller/pkg/controller/nodes/array/event_recorder.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5528      +/-   ##
==========================================
- Coverage   60.97%   60.97%   -0.01%     
==========================================
  Files         794      794              
  Lines       51488    51537      +49     
==========================================
+ Hits        31397    31426      +29     
- Misses      17199    17217      +18     
- Partials     2892     2894       +2     
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.73% <ø> (+0.04%) ⬆️
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 68.03% <ø> (ø)
unittests-flyteidl 79.04% <ø> (ø)
unittests-flyteplugins 61.85% <ø> (ø)
unittests-flytepropeller 57.25% <59.25%> (-0.04%) ⬇️
unittests-flytestdlib 65.59% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wild-endeavor wild-endeavor marked this pull request as ready for review July 1, 2024 19:51
@wild-endeavor wild-endeavor merged commit fc1c92c into master Jul 2, 2024
48 of 50 checks passed
@wild-endeavor wild-endeavor deleted the upstream/node-event-update branch July 2, 2024 18:42
vlibov pushed a commit to vlibov/flyte that referenced this pull request Aug 16, 2024
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants