Added GateNode message #296

hamersaw · 2022-05-03T23:13:15Z

TL;DR

GateNodes are used to stop the execution of downstream nodes. Simple gate node conditions are manual signalling or sleeping for a duration.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

^^^

Tracking Issue

flyteorg/flyte#208

Follow-up issue

NA

Signed-off-by: Daniel Rammer <daniel@union.ai>

codecov · 2022-05-12T00:13:39Z

Codecov Report

Merging #296 (d5b8572) into master (6965b90) will decrease coverage by 0.10%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   75.57%   75.46%   -0.11%     
==========================================
  Files          18       18              
  Lines        1171     1174       +3     
==========================================
+ Hits          885      886       +1     
- Misses        235      237       +2     
  Partials       51       51

Flag	Coverage Δ
unittests	`75.11% <50.00%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
clients/go/admin/client.go	`86.91% <33.33%> (-1.55%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Daniel Rammer <daniel@union.ai>

katrogan · 2022-06-15T16:37:49Z

protos/flyteidl/service/signal.proto

+service SignalService {
+  // Fetches or creates a :ref:`ref_flyteidl.admin.Signal`.
+  rpc GetOrCreateSignal (flyteidl.admin.SignalGetOrCreateRequest) returns (flyteidl.admin.Signal) {
+    // Purposfully left out an HTTP API for this RPC call. This is meant to idempotently retrieve


hopefully one day we'll have authz!

Does FlyteAdmin have a scheme implemented already? Happy to add something existing. I'm sure starting from the ground up would be a bit out of scope for this work.

katrogan · 2022-06-15T16:40:10Z

protos/flyteidl/admin/signal.proto

+    string token = 3;
+
+    // Indicates a list of filters passed as string.
+    // More info on constructing filters : <Link>


nit: can we populate the <Link> with a sphinx reference

To be honest, I just copied the list proto from other messages. It doesn't look like any have a link populated. Does one exist? Otherwise I will just remove the link.

katrogan · 2022-06-15T16:41:09Z

protos/flyteidl/admin/signal.proto

+}
+
+// Signal encapsulates a unique identifier, associated metadata, and a value for a single Flyte
+// signal. Signals may exist either without a set value (representing a signal request) or with a


interesting, can i ask why use an implicit behavior and not two separate messages for clarity instead?

I thought it may be easier for the model transformers within FlyteAdmin, write one and use if everywhere. I think using a single table in FlyteAdmin with both the type and value is the correct approach, so emulating that in proto messages. There does not seem to be strong uniformity between the different model types. I am open to changing this if anyone feels strongly.

protos/flyteidl/core/workflow.proto

katrogan · 2022-06-15T16:45:25Z

protos/flyteidl/core/workflow.proto

+    string signal_id = 1;
+
+    // A type denoting the required value type for this signal.
+    LiteralType type = 2;


Are there broader docs for how this is used? I didn't see an issue as part of the PR and I don't think I quite follow how this eventual value is produced. Could there be a case where we want multiple values as part of a signal?

The best resource is probably the RFC. Currently the implementation is setup so FlytePropeller calls the GetOrCreate RPC endpoint while evaluating the GateNode during each iteration. The node only succeeds if the signal has a value set.

An external entity will set the signal value. For example, a user through the SetSignal RPC endpoint or the UI can set the signal with a post to the RESTful interface.

And I believe the LiteralType proto encapsulates a Collection, Map, etc. So we should be able to support a variety of variable types, including multiple values.

Signed-off-by: Daniel Rammer <daniel@union.ai>

wild-endeavor · 2022-08-12T20:41:52Z

protos/flyteidl/core/workflow.proto

@@ -68,6 +68,35 @@ message WorkflowNode {
    }
 }

+// SignalCondition represents a dependency on an existing signal.


Can you clarify what you mean by an existing signal?

I was probably caught up in the flyteadmin side; where a signal does not technically "exist", meaning it can be provided, until propeller processes the gatenode that contains the signal condition. I'll go ahead and remove this, it can be confusing.

Signed-off-by: Daniel Rammer <daniel@union.ai>

protos/flyteidl/service/signal.proto

Signed-off-by: Daniel Rammer <daniel@union.ai>

* added GateNode message Signed-off-by: Daniel Rammer <daniel@union.ai> * added signal service Signed-off-by: Daniel Rammer <daniel@union.ai> * fleshed out Signal service Signed-off-by: Daniel Rammer <daniel@union.ai> * updated signal service with a GetOrCreateSignal and SetSignal API Signed-off-by: Daniel Rammer <daniel@union.ai> * updated signal service api to use GetOrCreate semantics Signed-off-by: Daniel Rammer <daniel@union.ai> * added the ListSignals API Signed-off-by: Daniel Rammer <daniel@union.ai> * fixed SignalListResponse proto name Signed-off-by: Daniel Rammer <daniel@union.ai> * set HTTP API parameters Signed-off-by: Daniel Rammer <daniel@union.ai> * generated protos Signed-off-by: Daniel Rammer <daniel@union.ai> * documented GateNode Signed-off-by: Daniel Rammer <daniel@union.ai> * updated signal list API Signed-off-by: Daniel Rammer <daniel@union.ai> * filled out signal list api Signed-off-by: Daniel Rammer <daniel@union.ai> * addressing pr comments on docs Signed-off-by: Daniel Rammer <daniel@union.ai> * added an output variable name to the signal condition Signed-off-by: Daniel Rammer <daniel@union.ai> * reworded signal condition docs Signed-off-by: Daniel Rammer <daniel@union.ai> * added ApproveCondition to GateNode Signed-off-by: Daniel Rammer <daniel@union.ai> * removed authOpt Signed-off-by: Daniel Rammer <daniel@union.ai> * fixed types Signed-off-by: Daniel Rammer <daniel@union.ai> * updated doc_gen_deps to fix docs generation Signed-off-by: Daniel Rammer <daniel@union.ai> Signed-off-by: Daniel Rammer <daniel@union.ai>

added GateNode message

2bd2e09

Signed-off-by: Daniel Rammer <daniel@union.ai>

hamersaw mentioned this pull request May 4, 2022

Add support for GateNode with signal and sleep condition flyteorg/flytepropeller#436

Merged

8 tasks

hamersaw added 3 commits May 9, 2022 14:22

Merge branch 'master' into feature/gate-nodes

d49aa5d

Signed-off-by: Daniel Rammer <daniel@union.ai>

added signal service

214188f

Signed-off-by: Daniel Rammer <daniel@union.ai>

fleshed out Signal service

36e4040

Signed-off-by: Daniel Rammer <daniel@union.ai>

hamersaw mentioned this pull request May 12, 2022

Implement signal service for human-in-the-loop workflow orchestration flyteorg/flyteadmin#423

Merged

8 tasks

hamersaw added 10 commits May 12, 2022 10:50

updated signal service with a GetOrCreateSignal and SetSignal API

e0f7d8a

Signed-off-by: Daniel Rammer <daniel@union.ai>

updated signal service api to use GetOrCreate semantics

d6967ea

Signed-off-by: Daniel Rammer <daniel@union.ai>

added the ListSignals API

89e13ff

Signed-off-by: Daniel Rammer <daniel@union.ai>

fixed SignalListResponse proto name

4affa79

Signed-off-by: Daniel Rammer <daniel@union.ai>

set HTTP API parameters

22bce35

Signed-off-by: Daniel Rammer <daniel@union.ai>

generated protos

0a5e035

Signed-off-by: Daniel Rammer <daniel@union.ai>

Merge branch 'master' into feature/gate-nodes

3b6d0f6

Signed-off-by: Daniel Rammer <daniel@union.ai>

documented GateNode

2c07f85

Signed-off-by: Daniel Rammer <daniel@union.ai>

updated signal list API

829ec07

Signed-off-by: Daniel Rammer <daniel@union.ai>

filled out signal list api

10be031

Signed-off-by: Daniel Rammer <daniel@union.ai>

hamersaw marked this pull request as ready for review June 14, 2022 23:56

hamersaw requested review from EngHabu, katrogan and kumare3 June 14, 2022 23:56

katrogan reviewed Jun 15, 2022

View reviewed changes

hamersaw added 2 commits June 16, 2022 07:45

addressing pr comments on docs

6044d6f

Signed-off-by: Daniel Rammer <daniel@union.ai>

added an output variable name to the signal condition

fcce430

Signed-off-by: Daniel Rammer <daniel@union.ai>

wild-endeavor reviewed Aug 12, 2022

View reviewed changes

hamersaw added 4 commits August 15, 2022 10:18

reworded signal condition docs

e34dc42

Signed-off-by: Daniel Rammer <daniel@union.ai>

added ApproveCondition to GateNode

18f9ff3

Signed-off-by: Daniel Rammer <daniel@union.ai>

Merge branch 'master' into feature/gate-nodes

d58fb68

Signed-off-by: Daniel Rammer <daniel@union.ai>

removed authOpt

0f00abe

Signed-off-by: Daniel Rammer <daniel@union.ai>

wild-endeavor previously approved these changes Sep 30, 2022

View reviewed changes

Merge branch 'master' into feature/gate-nodes

75ff73a

Signed-off-by: Daniel Rammer <daniel@union.ai>

hamersaw dismissed wild-endeavor’s stale review via 75ff73a October 10, 2022 22:52

hamersaw requested review from wild-endeavor and katrogan October 10, 2022 22:58

wild-endeavor previously approved these changes Oct 10, 2022

View reviewed changes

eapolinario reviewed Oct 11, 2022

View reviewed changes

protos/flyteidl/service/signal.proto Outdated Show resolved Hide resolved

protos/flyteidl/service/signal.proto Outdated Show resolved Hide resolved

fixed types

d0014cc

Signed-off-by: Daniel Rammer <daniel@union.ai>

hamersaw dismissed wild-endeavor’s stale review via d0014cc October 11, 2022 15:11

hamersaw requested review from wild-endeavor and eapolinario October 11, 2022 15:19

hamersaw added 2 commits October 11, 2022 12:12

Merge branch 'master' into feature/gate-nodes

dc9605c

Signed-off-by: Daniel Rammer <daniel@union.ai>

updated doc_gen_deps to fix docs generation

30fed9f

Signed-off-by: Daniel Rammer <daniel@union.ai>

eapolinario approved these changes Oct 13, 2022

View reviewed changes

wild-endeavor approved these changes Oct 13, 2022

View reviewed changes

Merge branch 'master' into feature/gate-nodes

d5b8572

EngHabu approved these changes Oct 25, 2022

View reviewed changes

EngHabu enabled auto-merge (squash) October 25, 2022 20:05

EngHabu disabled auto-merge October 25, 2022 20:05

EngHabu merged commit 71e934b into master Oct 25, 2022

EngHabu deleted the feature/gate-nodes branch October 25, 2022 20:05

wild-endeavor mentioned this pull request Dec 1, 2022

[Core feature] [flytekit] Add signaling experience to FlyteRemote flyteorg/flyte#3125

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added GateNode message #296

Added GateNode message #296

hamersaw commented May 3, 2022 •

edited by wild-endeavor

Loading

codecov bot commented May 12, 2022 •

edited

Loading

katrogan Jun 15, 2022

hamersaw Jun 16, 2022

katrogan Jun 15, 2022

hamersaw Jun 16, 2022

katrogan Jun 15, 2022

hamersaw Jun 16, 2022

katrogan Jun 15, 2022

hamersaw Jun 16, 2022

wild-endeavor Aug 12, 2022

hamersaw Aug 15, 2022

Added GateNode message #296

Added GateNode message #296

Conversation

hamersaw commented May 3, 2022 • edited by wild-endeavor Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented May 12, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamersaw commented May 3, 2022 •

edited by wild-endeavor

Loading

codecov bot commented May 12, 2022 •

edited

Loading