Auto generate node ID #162

honnix · 2022-12-20T22:43:52Z

TL;DR

Auto generate node ID

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

In most cases, there is no need to specify a node ID that must be unique across one workflow. This PR makes it possible to auto-generate one if unspecified.

If node ID is not specified, it will be generated as w<random>-n0, w<random>-n1, etc. where random is a random integer generated per workflow build, and w<random> can be specified by a user; and to make a node readable, node name is inferred from class name.

Tracking Issue

Closes flyteorg/flyte#3185

Follow-up issue

NA

Signed-off-by: Hongxin Liang <[email protected]>

honnix · 2022-12-20T22:44:31Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkCondition.java

@@ -54,13 +54,6 @@ public SdkBranchNode apply(
      List<String> upstreamNodeIds,
      @Nullable SdkNodeMetadata metadata,
      Map<String, SdkBindingData> inputs) {
-    if (metadata != null) {


This feels unnecessarily strict.

I had to delete these because of calling SdkTransform withNameOverride(String name, boolean failOnDuplicate)

honnix · 2022-12-20T22:44:51Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkContainerTask.java

@@ -53,11 +53,6 @@ public String getType() {
    return "raw-container";
  }

-  /** Specifies task name. */
-  public String getName() {


Moved to SdkTransform.

Signed-off-by: Hongxin Liang <[email protected]>

honnix · 2022-12-20T22:47:42Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkNode.java

@@ -58,6 +58,6 @@ public SdkNode apply(String id, SdkTransform transform) {
    List<String> upstreamNodeIds =
        getOutputs().isEmpty() ? Collections.singletonList(getNodeId()) : Collections.emptyList();

-    return builder.applyInternal(id, transform, upstreamNodeIds, /*metadata=*/ null, getOutputs());


This is strictly a breaking change, but I'm not sure why we had applyInternal as protected, and here metadata is always null.

honnix · 2022-12-20T22:48:18Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkPartialTransform.java

@@ -114,6 +126,11 @@ public SdkTransform withTimeoutOverride(Duration timeout) {
    return new SdkPartialTransform(transform, fixedInputs, extraUpstreamNodeIds, mergedMetadata);
  }

+  @Override
+  public String getName() {
+    return transform.getName();


Delegate to transform to get real name.

honnix · 2022-12-20T22:49:17Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java

+    return applyInternal(/*nodeId=*/ null, transform, emptyList(), inputs);
+  }
+
+  SdkNode applyInternal(


Changed visibility to package only. Note that this is strictly a breaking change.

honnix · 2022-12-20T22:50:19Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java

+
+  private static String toNodeName(String name) {
+    String lastPart = name.substring(name.lastIndexOf('.') + 1);
+    return PATTERN.matcher(lastPart).replaceAll("$1-$2").toLowerCase().replaceAll("\\$", "-");


Basically camel to kebab, also placing $ for nested class.

Signed-off-by: Hongxin Liang <[email protected]>

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java

Signed-off-by: Hongxin Liang <[email protected]>

honnix · 2022-12-21T07:49:25Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java


  public SdkWorkflowBuilder() {
+    this("w" + ThreadLocalRandom.current().nextInt(1000, 10000) + "-");


Make sure we could support subworkflow later.

should we move from numbers to characters? numbers can be confusing, as they might mean something, having a hash of characters might seem more natural

I went for consistency because we use number for node already n0, n1` etc. This is a also bit simpler than generating random bytes. But I'm ok either way.

OK, I revised it to use alphabet as prefix. This will also decrease the chance of conflicting.

honnix · 2022-12-21T08:05:00Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkTransform.java

+    return withNameOverride(name, true);
+  }
+
+  SdkTransform withNameOverride(String name, boolean failOnDuplicate) {


Intentionally not opening this method to avoid exposing unnecessary methods to users.

Signed-off-by: Hongxin Liang <[email protected]>

ckiosidis · 2022-12-21T13:00:03Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkTransform.java

  public SdkTransform withNameOverride(String name) {
+    return withNameOverride(name, true);
+  }


this is exposed publicly? So now we have 2 ways to set the name? One though the withNameOverride and the other through the buillder.apply(name,...)?

buillder.apply(nodeId,...) is for setting node ID, not node name.

ckiosidis · 2022-12-21T13:01:13Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java

 import javax.annotation.Nullable;
 import org.flyte.api.v1.LiteralType;
 import org.flyte.api.v1.SimpleType;
 import org.flyte.api.v1.WorkflowTemplate;

 public class SdkWorkflowBuilder {
+
+  private static final Pattern PATTERN = Pattern.compile("([a-z])([A-Z]+)");


we allow for capitals? i thought that only lower case was allowed

This is to convert FooBar to foo-bar for example.

Signed-off-by: Hongxin Liang <[email protected]>

ckiosidis

looks ok to me 👍

RRap0so

In flytekit-python, it throws exception if not set and there's a method to convert to dns-compliant.

See:
https://github.com/flyteorg/flytekit/blob/master/flytekit/core/node.py#L28
https://github.com/flyteorg/flytekit/blob/master/flytekit/core/utils.py#L13

Should we align with the Python way? Fine either way for me just want to be sure if we're supposed to be as much aligned as possible or if we can have our own flavor.

honnix · 2022-12-23T14:26:49Z

Where should we throw? In case of an invalid node id? That is reasonable but seems to be better in a different PR.

narape · 2022-12-27T14:32:00Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java

+            .ints('a', 'z' + 1)
+            .limit(4)


Suggested change

.ints('a', 'z' + 1)

.limit(4)

.ints(4, 'a', 'z' + 1)

ints has an overloaded version that take stream size as parameter, so no need for limit after

Maybe also add a constant RND_PREFIX_SIZE so it is easy to spot what that 4 means.
We could also add a comment to the method saying like

// Returns random prefix in the following format "wqjoz-"

narape · 2022-12-27T14:46:28Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java

+
+  private static String toNodeName(String name) {
+    String lastPart = name.substring(name.lastIndexOf('.') + 1);
+    return PATTERN


PATTERN is such a generic name specially in this class is about Workflow Builder.

I would suggest to either keeping the name as it is but move all the name and id related methods to a new auxiliary class, or try to come up with a more specific name for this constant. the challenge is that this pattern is a small piece in the whole (match a lowercase char followed by an uppercase one).

In any case,

Suggested change

return PATTERN

return UPPER_AFTER_LOWER_PATTERN

at least describe what the pattern does, but still I feel that there is no much cohesion between these methods and constant with the rest of the class

narape · 2022-12-27T15:12:50Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java

@@ -201,4 +236,23 @@ public void output(String name, SdkBindingData value, String help) {
  public WorkflowTemplate toIdlTemplate() {
    return WorkflowTemplateIdl.ofBuilder(this);
  }
+


Suggested change

// Translates "FooBar" to "foo-bar"

To make it easier to follow

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java

narape · 2022-12-27T17:05:05Z

flytekit-java/src/main/java/org/flyte/flytekit/SdkPartialTransform.java

+      if (failOnDuplicate) {
+        throw e;
+      }
+      return this;


This is surprising for me, if a name is already set and check is false then we don't override the name (this method becomes no op).

My suggestion would be to create a method called withNameOverrideIfNotSet (or similar) and then write the method like this:

public SdkTransform withNameOverrideIfNotSet(String name) { if (metadata.name() != null) { return this; } return withNameOverride(name); }

Leaving the withNameOverride as it was original

honnix · 2022-12-27T22:35:41Z

Please feel free to take over and make necessary changes. I will not have time to work on this any time soon. Thank you.

Signed-off-by: Nelson Arapé <[email protected]>

ckiosidis

looks good

* Auto generate node ID Signed-off-by: Hongxin Liang <[email protected]> * Do not expose Signed-off-by: Hongxin Liang <[email protected]> * OK Signed-off-by: Hongxin Liang <[email protected]> * Use remote name Signed-off-by: Hongxin Liang <[email protected]> * Use node id prefix Signed-off-by: Hongxin Liang <[email protected]> * Give it a proper prefix Signed-off-by: Hongxin Liang <[email protected]> * Use alphabet for prefix Signed-off-by: Hongxin Liang <[email protected]> * Node id related methods in own class Signed-off-by: Nelson Arapé <[email protected]> * withNameOverride(String, boolean) -> withNameOverrideIfNotSet Signed-off-by: Nelson Arapé <[email protected]> * Rename NamePolicy and add docs Signed-off-by: Nelson Arapé <[email protected]> * Minor refactor test Signed-off-by: Nelson Arapé <[email protected]> Signed-off-by: Hongxin Liang <[email protected]> Signed-off-by: Nelson Arapé <[email protected]> Co-authored-by: Nelson Arapé <[email protected]> Signed-off-by: Andres Gomez Ferrer <[email protected]>

Auto generate node ID

320616e

Signed-off-by: Hongxin Liang <[email protected]>

honnix commented Dec 20, 2022

View reviewed changes

Do not expose

e0d85d3

Signed-off-by: Hongxin Liang <[email protected]>

honnix commented Dec 20, 2022

View reviewed changes

OK

0fdb499

Signed-off-by: Hongxin Liang <[email protected]>

honnix commented Dec 20, 2022

View reviewed changes

flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java Show resolved Hide resolved

honnix added 2 commits December 21, 2022 08:22

Use remote name

7cb9900

Signed-off-by: Hongxin Liang <[email protected]>

Use node id prefix

c3216bd

Signed-off-by: Hongxin Liang <[email protected]>

honnix commented Dec 21, 2022

View reviewed changes

Give it a proper prefix

812fb04

Signed-off-by: Hongxin Liang <[email protected]>

ckiosidis reviewed Dec 21, 2022

View reviewed changes

Use alphabet for prefix

c6c6f38

Signed-off-by: Hongxin Liang <[email protected]>

honnix marked this pull request as ready for review December 23, 2022 10:13

ckiosidis approved these changes Dec 23, 2022

View reviewed changes

RRap0so reviewed Dec 23, 2022

View reviewed changes

narape reviewed Dec 27, 2022

View reviewed changes

narape added 4 commits December 28, 2022 11:41

Node id related methods in own class

1f965e6

Signed-off-by: Nelson Arapé <[email protected]>

withNameOverride(String, boolean) -> withNameOverrideIfNotSet

e2456e4

Signed-off-by: Nelson Arapé <[email protected]>

Rename NamePolicy and add docs

44b6d73

Signed-off-by: Nelson Arapé <[email protected]>

Minor refactor test

2b248eb

Signed-off-by: Nelson Arapé <[email protected]>

narape force-pushed the generate-node-id branch from cec276e to 2b248eb Compare December 28, 2022 12:48

ckiosidis approved these changes Dec 29, 2022

View reviewed changes

narape merged commit 993ce77 into master Dec 29, 2022

narape deleted the generate-node-id branch December 29, 2022 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto generate node ID #162

Auto generate node ID #162

honnix commented Dec 20, 2022 •

edited

Loading

honnix Dec 20, 2022

honnix Dec 21, 2022

honnix Dec 20, 2022

honnix Dec 20, 2022

honnix Dec 20, 2022

honnix Dec 20, 2022

honnix Dec 20, 2022

honnix Dec 21, 2022

ckiosidis Dec 21, 2022

honnix Dec 21, 2022

honnix Dec 23, 2022

honnix Dec 21, 2022

ckiosidis Dec 21, 2022

honnix Dec 21, 2022

ckiosidis Dec 21, 2022

honnix Dec 21, 2022

ckiosidis left a comment

RRap0so left a comment •

edited

Loading

honnix commented Dec 23, 2022

narape Dec 27, 2022

narape Dec 27, 2022

narape Dec 27, 2022

narape Dec 27, 2022

narape Dec 27, 2022

honnix commented Dec 27, 2022

ckiosidis left a comment


		public SdkWorkflowBuilder() {
		this("w" + ThreadLocalRandom.current().nextInt(1000, 10000) + "-");

Auto generate node ID #162

Auto generate node ID #162

Conversation

honnix commented Dec 20, 2022 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

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

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

ckiosidis left a comment

Choose a reason for hiding this comment

RRap0so left a comment • edited Loading

Choose a reason for hiding this comment

honnix commented Dec 23, 2022

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

honnix commented Dec 27, 2022

ckiosidis left a comment

Choose a reason for hiding this comment

honnix commented Dec 20, 2022 •

edited

Loading

RRap0so left a comment •

edited

Loading