Support remote in dynamic #248

honnix · 2023-09-27T21:24:09Z

TL;DR

This is an attempt to support remote tasks/launch plans in dynamic workflows.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

I don't see a good reason why we don't support the remote in dynamic setup, while we already have code doing the latest lookup etc.

Tracking Issue

NA

Follow-up issue

NA

honnix · 2023-09-27T21:29:40Z

jflyte/src/main/java/org/flyte/jflyte/ExecuteDynamicWorkflow.java

-
-    @Override
-    protected PartialTaskIdentifier visitTaskIdentifier(PartialTaskIdentifier value) {
-      if (value.project() == null && value.domain() == null && value.version() == null) {


So the current logic is, we only support task without any pdv coordinate, which means tasks in the workflow definition itself. For a remote entity, we do have pd(v).

Take an example from IdentifierRewrite.

PartialTaskIdentifier apply(PartialTaskIdentifier taskId) { String name = Preconditions.checkNotNull(taskId.name(), "name is null"); String project = coalesce(taskId.project(), project()); String domain = coalesce(taskId.domain(), domain()); String version = coalesce( taskId.version(), () -> taskId.project() == null ? version() : getLatestTaskVersion( /* project= */ project, /* domain= */ domain, /* name= */ name)); return PartialTaskIdentifier.builder() .name(name) .domain(domain) .project(project) .version(version) .build(); }

This means:

we take task name from task id, which is the same as DynamicWorkflowIdentifierRewrite

we take project if task id has it and in this case it is a remote one; but if it is a task in the workflow itself, this should be null, and this should cover the existing path in DynamicWorkflowIdentifierRewrite

the same applies to domain

we take version if task id has it and in this case it is a remote one with resolved (or hardcoded) version; but if it is a task in the workflow itself, this should be null and the same for project, so this should cover the existing path in DynamicWorkflowIdentifierRewrite; but if project is not null we will try to do a latest lookup to resolve the remote one

With this, we should both support local and remote tasks. The same applies to remote launch plan, and subworkflow.

There might be some corner cases that we may eventually need to write a new visitor, but I honestly don't see any blocker.

integration-tests/src/test/java/org/flyte/utils/JFlyteContainer.java

honnix · 2023-09-28T10:02:11Z

flytekit-examples/src/main/java/org/flyte/examples/DynamicFibonacciWorkflowTask.java

+      SdkNode<Void> hello =
+          builder.apply(
+              "hello",
+              SdkRemoteTask.create(


This is the remote task we reference from a dynamic workflow task.

honnix · 2023-09-28T10:02:39Z

flytekit-examples/src/main/java/org/flyte/examples/DynamicFibonacciWorkflowTask.java

-            builder.apply("fib-" + i, new SumTask(), SumInput.create(value, prev)).getOutputs();
+            builder
+                .apply(
+                    "fib-" + i, new SumTask().withUpstreamNode(hello), SumInput.create(value, prev))


Just to make a connection for fun, as it is not required.

honnix · 2023-09-28T10:03:01Z

jflyte/src/main/java/org/flyte/jflyte/ExecuteDynamicWorkflow.java

      DynamicJobSpec spec,
      Map<TaskIdentifier, TaskTemplate> taskTemplates,
      Map<WorkflowIdentifier, WorkflowTemplate> workflowTemplates) {

-    DynamicWorkflowIdentifierRewrite rewrite = new DynamicWorkflowIdentifierRewrite(config);
+    WorkflowNodeVisitor workflowNodeVisitor =


This is the core change.

honnix · 2023-09-28T11:52:47Z

integration-tests/src/test/java/org/flyte/utils/FlyteSandboxContainer.java

@@ -33,7 +33,7 @@

 public class FlyteSandboxContainer extends GenericContainer<FlyteSandboxContainer> {

-  public static final String IMAGE_NAME = "ghcr.io/flyteorg/flyte-sandbox:v1.1.0";
+  public static final String IMAGE_NAME = "ghcr.io/flyteorg/flyte-sandbox:v1.9.1";


Not required by this PR, but it's nice to be on latest version.

honnix · 2023-09-28T11:54:34Z

jflyte/pom.xml

@@ -200,7 +200,7 @@
            <environment>
              <FLYTE_INTERNAL_MODULE_DIR>/jflyte/modules</FLYTE_INTERNAL_MODULE_DIR>
              <FLYTE_INTERNAL_IMAGE>${docker.image}:${docker.tag}</FLYTE_INTERNAL_IMAGE>
-              <FLYTE_PLATFORM_URL>flyte:30081</FLYTE_PLATFORM_URL>
+              <FLYTE_PLATFORM_URL>flyteadmin.flyte.svc.cluster.local:81</FLYTE_PLATFORM_URL>


This is required for remote lookup to work because the task container running in k3 will need to talk to flyteadmin and using service DNS name is the best option.

Strictly speaking this is a breaking change but it is extremely unlikely that the existing config flyte:30081 would work for any users anyway and they have likely already customized it.

honnix · 2023-09-28T12:49:55Z

flytekit-testing/src/main/java/org/flyte/flytekit/testing/SdkTestingExecutor.java

+    return withTaskOutput0(task, input, output);
+  }
+
+  public <InputT, OutputT> SdkTestingExecutor withTaskOutput(


The diff is weird, but there is no change made to withTaskOutput(SdkRemoteTask, ...),

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

honnix commented Sep 27, 2023

View reviewed changes

honnix commented Sep 28, 2023

View reviewed changes

integration-tests/src/test/java/org/flyte/utils/JFlyteContainer.java Outdated Show resolved Hide resolved

honnix force-pushed the remote-in-dynamic branch from b41ae51 to 4263190 Compare September 28, 2023 09:45

honnix marked this pull request as ready for review September 28, 2023 10:00

honnix commented Sep 28, 2023

View reviewed changes

honnix mentioned this pull request Sep 28, 2023

reuse the identifierrewrite class #71

Closed

8 tasks

honnix commented Sep 28, 2023

View reviewed changes

honnix force-pushed the remote-in-dynamic branch from ac416d9 to 1db25b8 Compare September 28, 2023 12:47

honnix commented Sep 28, 2023

View reviewed changes

honnix added 9 commits September 28, 2023 14:56

Support remote in dynamic

8b95d48

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

Create flyteadmin client

fc22187

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

Use Config

ba3abc4

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

Fix import

04fea71

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

Integration test it

4f52b36

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

2023

eb0af74

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

Support testing dynamic workflow task

848dc42

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

Maybe a better diff

fa0176b

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

Restore pom.xml

d87ab1b

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

honnix force-pushed the remote-in-dynamic branch from d209362 to d87ab1b Compare September 28, 2023 12:56

andresgomezfrr approved these changes Sep 28, 2023

View reviewed changes

honnix merged commit 9b56702 into master Sep 28, 2023
4 checks passed

honnix deleted the remote-in-dynamic branch September 28, 2023 15:21

honnix mentioned this pull request Sep 29, 2023

Fetch task template in dynamic workflow task #254

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support remote in dynamic #248

Support remote in dynamic #248

honnix commented Sep 27, 2023 •

edited

honnix Sep 27, 2023

honnix Sep 27, 2023

honnix Sep 27, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

Support remote in dynamic #248

Support remote in dynamic #248

Conversation

honnix commented Sep 27, 2023 • edited

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

honnix commented Sep 27, 2023 •

edited