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 2 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
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
44 changes: 37 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 @@ -27,17 +27,24 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
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 AtomicInteger nodeIdSuffix;

public SdkWorkflowBuilder() {
// Using LinkedHashMap to preserve declaration order
Expand All @@ -47,34 +54,52 @@ public SdkWorkflowBuilder() {

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

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);
}

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

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, () -> "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 =
narape marked this conversation as resolved.
Show resolved Hide resolved
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 +226,9 @@ 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.matcher(lastPart).replaceAll("$1-$2").toLowerCase().replaceAll("\\$", "-");
Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
}
63 changes: 50 additions & 13 deletions flytekit-java/src/test/java/org/flyte/flytekit/SdkWorkflowBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,18 @@
class SdkWorkflowBuilderTest {

@Test
void testTimes2WorkflowIdl() {
void testTimes4WorkflowIdl() {
SdkWorkflowBuilder builder = new SdkWorkflowBuilder();

new Times2Workflow().expand(builder);
new Times4Workflow().expand(builder);

Node expectedNode =
Node node0 =
Node.builder()
.id("square")
.id("n0")
.metadata(
NodeMetadata.builder()
.name("sdk-workflow-builder-test-multiplication-task")
.build())
.taskNode(
TaskNode.builder()
.referenceId(
Expand All @@ -90,13 +94,42 @@ void testTimes2WorkflowIdl() {
.build()))
.upstreamNodeIds(emptyList())
.build();
Node node1 =
Node.builder()
.id("n1")
.metadata(
NodeMetadata.builder()
.name("sdk-workflow-builder-test-multiplication-task")
.build())
.taskNode(
TaskNode.builder()
.referenceId(
PartialTaskIdentifier.builder()
.name("org.flyte.flytekit.SdkWorkflowBuilderTest$MultiplicationTask")
.build())
.build())
.inputs(
asList(
Binding.builder()
.var_("a")
.binding(
BindingData.ofOutputReference(
OutputReference.builder().var("c").nodeId("n0").build()))
.build(),
Binding.builder()
.var_("b")
.binding(
BindingData.ofScalar(Scalar.ofPrimitive(Primitive.ofIntegerValue(2L))))
.build()))
.upstreamNodeIds(emptyList())
.build();

WorkflowTemplate expected =
WorkflowTemplate.builder()
.metadata(WorkflowMetadata.builder().build())
.interface_(expectedInterface())
.outputs(expectedOutputs())
.nodes(singletonList(expectedNode))
.outputs(expectedOutputs("n1"))
.nodes(List.of(node0, node1))
.build();

assertEquals(expected, builder.toIdlTemplate());
Expand Down Expand Up @@ -176,7 +209,7 @@ void testConditionalWorkflowIdl() {
WorkflowTemplate.builder()
.metadata(WorkflowMetadata.builder().build())
.interface_(expectedInterface())
.outputs(expectedOutputs())
.outputs(expectedOutputs("square"))
.nodes(singletonList(expectedNode))
.build();

Expand Down Expand Up @@ -445,28 +478,32 @@ private TypedInterface expectedInterface() {
.build();
}

private List<Binding> expectedOutputs() {
private List<Binding> expectedOutputs(String nodeId) {
return singletonList(
Binding.builder()
.var_("out")
.binding(
BindingData.ofOutputReference(
OutputReference.builder().var("c").nodeId("square").build()))
OutputReference.builder().var("c").nodeId(nodeId).build()))
.build());
}

private static class Times2Workflow extends SdkWorkflow {
private static class Times4Workflow extends SdkWorkflow {

@Override
public void expand(SdkWorkflowBuilder builder) {
SdkBindingData in = builder.inputOfInteger("in", "Enter value to square");
SdkBindingData two = literalOfInteger(2L);
SdkBindingData out =
SdkBindingData out1 =
builder
.apply(new MultiplicationTask().withInput("a", in).withInput("b", two))
.getOutput("c");
SdkBindingData out2 =
builder
.apply("square", new MultiplicationTask().withInput("a", in).withInput("b", two))
.apply(new MultiplicationTask().withInput("a", out1).withInput("b", two))
.getOutput("c");

builder.output("out", out);
builder.output("out", out2);
}
}

Expand Down
2 changes: 2 additions & 0 deletions flytekit-java/src/test/java/org/flyte/flytekit/SdkWorkflowWithSdkRemoteLaunchPlanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.flyte.api.v1.BindingData;
import org.flyte.api.v1.Literal;
import org.flyte.api.v1.Node;
import org.flyte.api.v1.NodeMetadata;
import org.flyte.api.v1.OutputReference;
import org.flyte.api.v1.PartialLaunchPlanIdentifier;
import org.flyte.api.v1.TypedInterface;
Expand All @@ -48,6 +49,7 @@ void applyShouldReturnASdkWorkflowNode() {
Node expectedNode =
Node.builder()
.id("some-node-id")
.metadata(NodeMetadata.builder().name("some-node-id").build())
.workflowNode(
WorkflowNode.builder()
.reference(
Expand Down