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

fix: this is rfc, do not deploy until resolved; details in comments #172

Merged
merged 3 commits into from
Sep 13, 2021

Conversation

jsonporter
Copy link
Contributor

@jsonporter jsonporter commented Jun 7, 2021

RFC: DO NOT DEPLOY UNTIL RESOLVED

I am working on the fix for flyteorg/flyte#1085 and have identified where the workflow breaks. This change fixes the issue (short version: id's were being used as keys but their values were truncated and thus we had duplicates) however I am new to the code base and unclear on the implications of removing this ternary. Where/why do we use metadata rather than id?

Can someone with more context please take a look and help me understand if there are any edge cases I am missing here?

Type

  • [ X] Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

flyteorg/flyte#1085

@jsonporter jsonporter requested a review from schottra June 7, 2021 16:38
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@f2ce32d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #172   +/-   ##
=========================================
  Coverage          ?   70.28%           
=========================================
  Files             ?      340           
  Lines             ?     6747           
  Branches          ?     1126           
=========================================
  Hits              ?     4742           
  Misses            ?     2005           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2ce32d...d367f9b. Read the comment docs.

@jsonporter jsonporter force-pushed the flyteconsole-nested-workflow-bug branch from ecb55bd to 8955aab Compare June 7, 2021 16:46
note: deploying with caution; the believe if that it was incorrect to use
meta.

Signed-off-by: Jason Porter <[email protected]>
@schottra
Copy link
Contributor

@jsonporter I could be wrong, but I believe this is a case of backwards-compatibility. For complex nested workflows, we may end up executing the same node multiple times. Each node execution will have a dynamically-generated id. But it will point to a node in the workflow spec by using metadata.specNodeId. However, we aren't guaranteed that the specNodeId will be set, so we fall back to node.id. I believe by removing the usage of metadata, we would break certain use cases of dynamic execution where a node is run multiple times.
cc @kumare3 and @anandswaminathan for more context.

@kumare3
Copy link
Contributor

kumare3 commented Jul 14, 2021

@jsonporter whats the status on this one?

@flyte-bot
Copy link
Collaborator

🎉 This PR is included in version 0.25.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

jsonporter added a commit that referenced this pull request Sep 17, 2021
…172)

* fix: this is rfc, do not deploy until resolved; details in comments

Signed-off-by: Jason Porter <[email protected]>

* fix: this is rfc, do not deploy until resolved; details in comments

note: deploying with caution; the believe if that it was incorrect to use
meta.

Signed-off-by: Jason Porter <[email protected]>
jsonporter added a commit that referenced this pull request Sep 17, 2021
…172)

* fix: this is rfc, do not deploy until resolved; details in comments

Signed-off-by: Jason Porter <[email protected]>

* fix: this is rfc, do not deploy until resolved; details in comments

note: deploying with caution; the believe if that it was incorrect to use
meta.

Signed-off-by: Jason Porter <[email protected]>
jsonporter added a commit that referenced this pull request Sep 17, 2021
* feat: add workflow versions table

Signed-off-by: csirius <[email protected]>
Signed-off-by: Jason Porter <[email protected]>

* chore(release): Release 0.25.0 [skip ci]

# [0.25.0](http://github.com/lyft/flyteconsole/compare/v0.24.0...v0.25.0) (2021-08-31)

### Features

* add workflow versions table ([#193](http://github.com/lyft/flyteconsole/issues/193)) ([6fff87e](http://github.com/lyft/flyteconsole/commit/6fff87e40007fd15faae634eb6402045c067dd2c))

Signed-off-by: Jason Porter <[email protected]>

* improvement: show proper error message for aborted workflows (#195)

* improvement: show proper error message for aborted workflows

Signed-off-by: Pianist038801 <[email protected]>

* improvement: show abort message in the execution list

Signed-off-by: Pianist038801 <[email protected]>

Co-authored-by: Pianist038801 <[email protected]>
Signed-off-by: Jason Porter <[email protected]>

* Fix/versions executions gap (#197)

* feat: add workflow versions table

Signed-off-by: csirius <[email protected]>

* fix: space between versions and executions table

Signed-off-by: csirius <[email protected]>
Signed-off-by: Jason Porter <[email protected]>

* feat: workflow version details page

Signed-off-by: csirius <[email protected]>
Signed-off-by: Jason Porter <[email protected]>

* Pre merge checkin

Signed-off-by: Jason Porter <[email protected]>

* fix: this is rfc, do not deploy until resolved; details in comments (#172)

* fix: this is rfc, do not deploy until resolved; details in comments

Signed-off-by: Jason Porter <[email protected]>

* fix: this is rfc, do not deploy until resolved; details in comments

note: deploying with caution; the believe if that it was incorrect to use
meta.

Signed-off-by: Jason Porter <[email protected]>

* fix: show field types on json launch form (#199)

Signed-off-by: Pianist038801 <[email protected]>

Co-authored-by: Pianist038801 <[email protected]>
Signed-off-by: Jason Porter <[email protected]>

* chore(release): Release 0.25.1 [skip ci]

## [0.25.1](http://github.com/lyft/flyteconsole/compare/v0.25.0...v0.25.1) (2021-09-13)

### Bug Fixes

* show field types on json launch form ([#199](http://github.com/lyft/flyteconsole/issues/199)) ([a42b9f8](http://github.com/lyft/flyteconsole/commit/a42b9f8520fcd24dee752111e606ad9ae9bd88f5))
* this is rfc, do not deploy until resolved; details in comments ([#172](http://github.com/lyft/flyteconsole/issues/172)) ([67dd183](http://github.com/lyft/flyteconsole/commit/67dd18397caf40e350da40e0672e500eaa9f338a))

Signed-off-by: Jason Porter <[email protected]>

* Minor fixes

Signed-off-by: Jason Porter <[email protected]>

* Feat/version details (#198)

* feat: add workflow versions table

Signed-off-by: csirius <[email protected]>

* feat: workflow version details page

Signed-off-by: csirius <[email protected]>
Signed-off-by: Jason Porter <[email protected]>

* fix: repopulate struct input fields on relaunch form (#201)

Signed-off-by: Pianist038801 <[email protected]>

Co-authored-by: Pianist038801 <[email protected]>
Signed-off-by: Jason Porter <[email protected]>

* Graph ux feature add legend (#196)

* Checkin and merge to master

Signed-off-by: Jason Porter <[email protected]>

* fixed one more

Signed-off-by: Jason Porter <[email protected]>

* chore(release): Release 0.25.2 [skip ci]

## [0.25.2](http://github.com/lyft/flyteconsole/compare/v0.25.1...v0.25.2) (2021-09-16)

### Bug Fixes

* repopulate struct input fields on relaunch form ([#201](http://github.com/lyft/flyteconsole/issues/201)) ([950e080](http://github.com/lyft/flyteconsole/commit/950e080a2c52c6294630f0ffd1410b0e45b50a8d))

Signed-off-by: Jason Porter <[email protected]>

* final before pr

Signed-off-by: Jason Porter <[email protected]>

* Fixed issues created by bad rebase

Signed-off-by: Jason Porter <[email protected]>

* Fixed more merge issues

Signed-off-by: Jason Porter <[email protected]>

Co-authored-by: csirius <[email protected]>
Co-authored-by: flyte bot <[email protected]>
Co-authored-by: pianist <[email protected]>
Co-authored-by: Pianist038801 <[email protected]>
Co-authored-by: csirius <[email protected]>
@anrusina anrusina deleted the flyteconsole-nested-workflow-bug branch January 13, 2022 00:24
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.

5 participants