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.

Sign up for GitHub

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

Auto generate node ID #162

Merged
merged 11 commits into from
Dec 29, 2022
Merged

Auto generate node ID #162

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
320616e
Auto generate node ID
honnix Dec 20, 2022
e0d85d3
Do not expose
honnix Dec 20, 2022
0fdb499
OK
honnix Dec 20, 2022
7cb9900
Use remote name
honnix Dec 21, 2022
c3216bd
Use node id prefix
honnix Dec 21, 2022
812fb04
Give it a proper prefix
honnix Dec 21, 2022
c6c6f38
Use alphabet for prefix
honnix Dec 23, 2022
1f965e6
Node id related methods in own class
narape Dec 28, 2022
e2456e4
withNameOverride(String, boolean) -> withNameOverrideIfNotSet
narape Dec 28, 2022
44b6d73
Rename NamePolicy and add docs
narape Dec 28, 2022
2b248eb
Minor refactor test
narape Dec 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,6 @@ public SdkBranchNode apply(
List<String> upstreamNodeIds,
@Nullable SdkNodeMetadata metadata,
Map<String, SdkBindingData> inputs) {
if (metadata != null) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels unnecessarily strict.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new IllegalArgumentException("invariant failed: metadata must be null");
}
if (!inputs.isEmpty()) {
throw new IllegalArgumentException("invariant failed: inputs must be empty");
}

SdkBranchNode.Builder nodeBuilder = new SdkBranchNode.Builder(builder);

for (SdkConditionCase case_ : cases) {
Expand Down
5 changes: 0 additions & 5 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkContainerTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@ public String getType() {
return "raw-container";
}

/** Specifies task name. */
public String getName() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to SdkTransform.

return getClass().getName();
}

/** Specifies task input type. */
public SdkType<InputT> getInputType() {
return inputType;
Expand Down
4 changes: 0 additions & 4 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkDynamicWorkflowTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ public String getType() {
return "dynamic";
}

public String getName() {
return getClass().getName();
}

public SdkType<InputT> getInputType() {
return inputType;
}
Expand Down
2 changes: 1 addition & 1 deletion flytekit-java/src/main/java/org/flyte/flytekit/SdkNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return builder.applyInternal(id, transform, upstreamNodeIds, getOutputs());
}
}
19 changes: 18 additions & 1 deletion flytekit-java/src/main/java/org/flyte/flytekit/SdkPartialTransform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,22 @@ public SdkTransform withUpstreamNode(SdkNode node) {

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

@Override
SdkTransform withNameOverride(String name, boolean failOnDuplicate) {
requireNonNull(name, "Name override cannot be null");

SdkNodeMetadata newMetadata = SdkNodeMetadata.builder().name(name).build();
checkForDuplicateMetadata(metadata, newMetadata, SdkNodeMetadata::name, "name");
try {
checkForDuplicateMetadata(metadata, newMetadata, SdkNodeMetadata::name, "name");
} catch (IllegalArgumentException e) {
if (failOnDuplicate) {
throw e;
}
return this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
SdkNodeMetadata mergedMetadata = mergeMetadata(metadata, newMetadata);

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

@Override
public String getName() {
return transform.getName();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delegate to transform to get real name.

}

@Override
public SdkNode apply(
SdkWorkflowBuilder builder,
Expand Down
5 changes: 5 additions & 0 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkRemoteLaunchPlan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ public static <InputT, OutputT> SdkRemoteLaunchPlan<InputT, OutputT> create(
.build();
}

@Override
public String getName() {
return name();
}

@Override
public SdkNode apply(
SdkWorkflowBuilder builder,
Expand Down
5 changes: 5 additions & 0 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkRemoteTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ public static <InputT, OutputT> SdkRemoteTask<InputT, OutputT> create(
.build();
}

@Override
public String getName() {
return name();
}

@Override
public SdkNode apply(
SdkWorkflowBuilder builder,
Expand Down
4 changes: 0 additions & 4 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkRunnableTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,6 @@ public String getType() {
return "java-task";
}

public String getName() {
return getClass().getName();
}

public SdkType<InputT> getInputType() {
return inputType;
}
Expand Down
8 changes: 8 additions & 0 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkTransform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ public SdkTransform withUpstreamNode(SdkNode node) {
}

public SdkTransform withNameOverride(String name) {
return withNameOverride(name, true);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,...)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


SdkTransform withNameOverride(String name, boolean failOnDuplicate) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

requireNonNull(name, "Name override cannot be null");

SdkNodeMetadata metadata = SdkNodeMetadata.builder().name(name).build();
Expand All @@ -84,4 +88,8 @@ public SdkTransform withTimeoutOverride(Duration timeout) {
SdkNodeMetadata metadata = SdkNodeMetadata.builder().timeout(timeout).build();
return SdkPartialTransform.of(this, metadata);
}

public String getName() {
return getClass().getName();
}
}
4 changes: 0 additions & 4 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@

public abstract class SdkWorkflow extends SdkTransform {

public String getName() {
return getClass().getName();
}

public abstract void expand(SdkWorkflowBuilder builder);

@Override
Expand Down
68 changes: 61 additions & 7 deletions flytekit-java/src/main/java/org/flyte/flytekit/SdkWorkflowBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,55 +26,90 @@
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.regex.Pattern;
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]+)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private final Map<String, SdkNode> nodes;
private final Map<String, SdkBindingData> inputs;
private final Map<String, SdkBindingData> outputs;
private final Map<String, String> inputDescriptions;
private final Map<String, String> outputDescriptions;
private final String nodeIdPrefix;
private final AtomicInteger nodeIdSuffix;

public SdkWorkflowBuilder() {
this(randomPrefix());
}

// VisibleForTesting
SdkWorkflowBuilder(String nodeIdPrefix) {
// Using LinkedHashMap to preserve declaration order
this.nodes = new LinkedHashMap<>();
this.inputs = new LinkedHashMap<>();
this.outputs = new LinkedHashMap<>();

this.inputDescriptions = new HashMap<>();
this.outputDescriptions = new HashMap<>();

this.nodeIdPrefix = nodeIdPrefix;
this.nodeIdSuffix = new AtomicInteger();
}

public SdkNode apply(String nodeId, SdkTransform transform) {
return apply(nodeId, transform, emptyMap());
}

public SdkNode apply(String nodeId, SdkTransform transform, Map<String, SdkBindingData> inputs) {
return applyInternal(nodeId, transform, emptyList(), /*metadata=*/ null, inputs);
return applyInternal(nodeId, transform, emptyList(), inputs);
}

public SdkNode apply(SdkTransform transform) {
return apply(/*nodeId=*/ null, transform, emptyMap());
}

protected SdkNode applyInternal(
String nodeId,
public SdkNode apply(SdkTransform transform, Map<String, SdkBindingData> inputs) {
return applyInternal(/*nodeId=*/ null, transform, emptyList(), inputs);
}

SdkNode applyInternal(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Nullable String nodeId,
SdkTransform transform,
List<String> upstreamNodeIds,
@Nullable SdkNodeMetadata metadata,
Map<String, SdkBindingData> inputs) {

if (nodes.containsKey(nodeId)) {
String actualNodeId =
Objects.requireNonNullElseGet(
nodeId, () -> nodeIdPrefix + "n" + nodeIdSuffix.getAndIncrement());

if (nodes.containsKey(actualNodeId)) {
CompilerError error =
CompilerError.create(
CompilerError.Kind.DUPLICATE_NODE_ID,
nodeId,
actualNodeId,
"Trying to insert two nodes with the same id.");

throw new CompilerException(error);
}

SdkNode sdkNode = transform.apply(this, nodeId, upstreamNodeIds, metadata, inputs);
String fallbackNodeName =
Objects.requireNonNullElseGet(nodeId, () -> toNodeName(transform.getName()));

SdkNode sdkNode =
transform
.withNameOverride(fallbackNodeName, false)
.apply(this, actualNodeId, upstreamNodeIds, null, inputs);
nodes.put(sdkNode.getNodeId(), sdkNode);

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

To make it easier to follow

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.matcher(lastPart)
.replaceAll("$1-$2")
.toLowerCase(Locale.ROOT)
.replaceAll("\\$", "-");
}

// VisibleForTesting
static String randomPrefix() {
return "w"
+ ThreadLocalRandom.current()
.ints('a', 'z' + 1)
.limit(4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-"

.collect(StringBuilder::new, StringBuilder::appendCodePoint, StringBuilder::append)
.append('-');
}
}
Loading