Blob and struct support #258

honnix · 2023-10-07T21:14:27Z

TL;DR

Restore the more or less useless blob support that we had before moving to 0.4. Also added struct support for both Java and Scala.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

It is more or less useless because blob is designed to go together with offloaded data (blob is not designed to be used directly as a task input/output) https://docs.flyte.org/projects/flytekit/en/latest/generated/flytekit.BlobType.html#flytekit-blobtype, which I think we have no plan to support any time soon.

Add struct support for both Java and Scala so a task can have nested input/output using either auto-value object or case class.

Note that originally this work was broken into 3 PRs (this and #259 and #262), each on top of a previous one, to avoid complex merging and conflict resolving. In a later stage of development, we merged #259 and #262 to this PR to ship everything in one goal.

Tracking Issue

NA

Follow-up issue

NA

honnix · 2023-10-09T08:35:43Z

flytekit-scala_2.13/src/main/scala/org/flyte/flytekitscala/SdkScalaType.scala

@@ -230,6 +231,18 @@ object SdkScalaType {
  implicit def durationLiteralType: SdkScalaLiteralType[Duration] =
    DelegateLiteralType(SdkLiteralTypes.durations())

+  // fixme: create blob type from annotation
+  implicit def blobLiteralType: SdkScalaLiteralType[Blob] =


I tried adding a new annotation for scala but it didn't work well because we will need to recreate a param, which is not easy. For the record, it is something like:

private def updateParamIfNeeded[T](param: Param[Typeclass, T]) = { param.annotations .collectFirst { case ann: BlobTypeDescription => SdkLiteralTypes .blobs( BlobType .builder() .format(ann.format) .dimensionality(ann.dimensionality) .build() ) .asInstanceOf[SdkLiteralType[Any]] } .map(sdkBindingLiteralType => // fixme: this apply function is not designed to be called from user code Param( param.label, param.typeName, param.index, param.repeated, CallByNeed(sdkBindingLiteralType), CallByNeed(param.default), param.annotationsArray, param.typeAnnotationsArray ) ) .getOrElse(param) } def combine[T](ctx: CaseClass[SdkScalaType, T]): SdkScalaProductType[T] = { // throwing an exception will abort implicit resolution for this case // very dirty down casting, but we need some evidence that all parameters are TypedLiterals // and that's the best we can do with magnolia unless we want to play with phantom types case class ParamsWithDesc( param: Param[SdkScalaLiteralType, T], desc: Option[String] ) val params = ctx.parameters.map { param => param.typeclass match { case _: SdkScalaProductType[_] => sys.error("nested structs aren't supported") case _: SdkScalaLiteralType[_] => val optDesc = param.annotations.collectFirst { case ann: Description => ann.value } val updatedParam = updateParamIfNeeded(param) ParamsWithDesc( updatedParam.asInstanceOf[Param[SdkScalaLiteralType, T]], optDesc ) } }

First of all, the apply is not supposed to be used; second, I got weird compilation problem that I didn't have time (and it didn't seem worth it either) to dig deeply.

honnix · 2023-10-09T08:38:17Z

flytekit-jackson/src/main/java/org/flyte/flytekit/jackson/VariableMapVisitor.java

-      return SdkLiteralTypes.maps(toLiteralType(valueType, false, propName, declaringClassName));
+      return SdkLiteralTypes.maps(toLiteralType(valueType, false, propName, member));
+    } else if (Blob.class.isAssignableFrom(type)) {
+      BlobTypeDescription annotation = member.getAnnotation(BlobTypeDescription.class);


This is the only reason we need to get hold of the whole member object.

honnix · 2023-10-09T08:38:54Z

...ckson/src/main/java/org/flyte/flytekit/jackson/deserializers/SdkBindingDataDeserializer.java

@@ -80,7 +83,7 @@ private SdkBindingData<?> transform(JsonNode tree) {
    }
  }

-  private static SdkBindingData<? extends Serializable> transformScalar(JsonNode tree) {


This is unnecessarily strict and blob is not serializable.

honnix · 2023-10-09T08:46:56Z

integration-tests/pom.xml

@@ -51,6 +51,11 @@
      <artifactId>auto-service-annotations</artifactId>
      <scope>provided</scope>
    </dependency>
+    <dependency>


This is needed because we now reference Blob in flytekit-java.

honnix · 2023-10-09T08:48:01Z

flytekit-scala_2.13/src/main/scala/org/flyte/flytekitscala/SdkBindingDataConverters.scala

@@ -172,7 +172,6 @@ object SdkBindingDataConverters {
              jf.Function.identity()
            )
        }
-      case LiteralType.Kind.BLOB_TYPE   => ??? // TODO not yet supported


There is no thing to covert for Blob because it is the same class for scala and java.

honnix · 2023-10-09T08:48:55Z

...ckson/src/main/java/org/flyte/flytekit/jackson/deserializers/SdkBindingDataDeserializer.java

      default:
        throw new UnsupportedOperationException(
            "Type contains an unsupported scalar: " + scalarKind);
    }
  }

+  private static SdkBindingData<Blob> transformBlob(JsonNode tree) {


This is the main change to get blob back.

honnix · 2023-10-09T21:33:31Z

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

@@ -48,8 +51,8 @@ public abstract static class AutoAllInputsInput {

    public abstract SdkBindingData<Duration> d();

-    // TODO add blobs to sdkbinding data
-    // public abstract SdkBindingData<Blob> blob();
+    @BlobTypeDescription(format = "csv", dimensionality = BlobDimensionality.MULTIPART)


I'm not entirely sure whether we should add this annotation because Blob type is not meant to be used by users directly. We could decide to restore to exactly what it was in 0.3.

I removed this in a later commit. My reasoning is to lower the surface area until we figured out whether we want to support https://docs.flyte.org/projects/flytekit/en/latest/generated/flytekit.BlobType.html#flytekit-blobtype property.

honnix · 2023-10-17T08:27:21Z

Note that #262 is also targeting blob branch.

honnix · 2023-10-17T08:30:20Z

~~I don't know what DCO is complaining about. Every commit is signed I believe.~~ Fixed it but I still don't know why :D.

honnix · 2023-10-17T08:46:12Z

Note that #262 is also targeting blob branch.

@andresgomezfrr Maybe let's just use this PR to ship everything including #262 ?

andresgomezfrr · 2023-10-17T10:43:27Z

Note that #262 is also targeting blob branch.

@andresgomezfrr Maybe let's just use this PR to ship everything including #262 ?

ok! I just reviewed this one, but before merge I'll review the scala one too

andresgomezfrr · 2023-10-17T12:28:44Z

@honnix we need to fix the DCO to merge this one

honnix · 2023-10-17T12:29:48Z

@honnix we need to fix the DCO to merge this one

Yeah. On it.

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

honnix · 2023-10-17T12:48:52Z

Let's ? @andresgomezfrr

honnix force-pushed the blob branch 8 times, most recently from 3280083 to 4d261c9 Compare October 8, 2023 21:15

honnix commented Oct 9, 2023

View reviewed changes

honnix marked this pull request as ready for review October 9, 2023 09:23

honnix force-pushed the blob branch 4 times, most recently from 6ea206b to 8284ee3 Compare October 9, 2023 21:09

honnix commented Oct 9, 2023

View reviewed changes

honnix force-pushed the blob branch from cc493fa to f6e7942 Compare October 10, 2023 11:10

honnix mentioned this pull request Oct 17, 2023

Support struct for Java #259

Merged

8 tasks

honnix force-pushed the blob branch from 78aea79 to de26c25 Compare October 17, 2023 08:31

andresgomezfrr approved these changes Oct 17, 2023

View reviewed changes

honnix added 7 commits October 17, 2023 14:30

Bring back Blob support

a320117

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

Remove BlobTypeDescription

7bf61d3

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

Add blob back

619b825

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

Blob in list and map

2ae90d7

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

Clean up

d61be65

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

Support struct (#259)

865f81b

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

Support struct in Scala layer (#262)

bf6d789

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

honnix force-pushed the blob branch from 76fb3be to bf6d789 Compare October 17, 2023 12:30

honnix changed the title ~~Bring back Blob support~~ Blob and struct support Oct 17, 2023

andresgomezfrr merged commit 6a26d59 into master Oct 17, 2023
3 checks passed

andresgomezfrr deleted the blob branch October 17, 2023 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blob and struct support #258

Blob and struct support #258

honnix commented Oct 7, 2023 •

edited

Loading

honnix Oct 9, 2023

honnix Oct 9, 2023

honnix Oct 9, 2023

honnix Oct 9, 2023

honnix Oct 9, 2023

honnix Oct 9, 2023

honnix Oct 9, 2023

honnix Oct 10, 2023 •

edited

Loading

honnix commented Oct 17, 2023

honnix commented Oct 17, 2023 •

edited

Loading

honnix commented Oct 17, 2023

andresgomezfrr commented Oct 17, 2023

andresgomezfrr commented Oct 17, 2023

honnix commented Oct 17, 2023

honnix commented Oct 17, 2023

Blob and struct support #258

Blob and struct support #258

Conversation

honnix commented Oct 7, 2023 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

honnix Oct 9, 2023

Choose a reason for hiding this comment

honnix Oct 9, 2023

Choose a reason for hiding this comment

honnix Oct 9, 2023

Choose a reason for hiding this comment

honnix Oct 9, 2023

Choose a reason for hiding this comment

honnix Oct 9, 2023

Choose a reason for hiding this comment

honnix Oct 9, 2023

Choose a reason for hiding this comment

honnix Oct 9, 2023

Choose a reason for hiding this comment

honnix Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

honnix commented Oct 17, 2023

honnix commented Oct 17, 2023 • edited Loading

honnix commented Oct 17, 2023

andresgomezfrr commented Oct 17, 2023

andresgomezfrr commented Oct 17, 2023

honnix commented Oct 17, 2023

honnix commented Oct 17, 2023

honnix commented Oct 7, 2023 •

edited

Loading

honnix Oct 10, 2023 •

edited

Loading

honnix commented Oct 17, 2023 •

edited

Loading