[SPARK-22666][ML][SQL] Spark datasource for image format #22328

WeichenXu123 · 2018-09-04T10:02:56Z

What changes were proposed in this pull request?

Implement an image schema datasource.

This image datasource support:

partition discovery (loading partitioned images)
dropImageFailures (the same behavior with ImageSchema.readImage)
path wildcard matching (the same behavior with ImageSchema.readImage)
loading recursively from directory (different from ImageSchema.readImage, but use such path: /path/to/dir/**)

This datasource NOT support:

specify numPartitions (it will be determined by datasource automatically)
sampling (you can use df.sample later but the sampling operator won't be pushdown to datasource)

How was this patch tested?

Unit tests.

Benchmark

I benchmark and compare the cost time between old ImageSchema.read API and my image datasource.

cluster: 4 nodes, each with 64GB memory, 8 cores CPU
test dataset: Flickr8k_Dataset (about 8091 images)

time cost:

My image datasource time (automatically generate 258 partitions): 38.04s
ImageSchema.read time (set 16 partitions): 68.4s
ImageSchema.read time (set 258 partitions): 90.6s

time cost when increase image number by double (clone Flickr8k_Dataset and loads double number images):

My image datasource time (automatically generate 515 partitions): 95.4s
ImageSchema.read (set 32 partitions): 109s
ImageSchema.read (set 515 partitions): 105s

So we can see that my image datasource implementation (this PR) bring some performance improvement compared against oldImageSchema.read API.

SparkQA · 2018-09-04T10:10:07Z

Test build #95667 has finished for PR 22328 at commit 5b5aee6.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-09-04T10:09:21Z

mllib/src/main/resources/META-INF/services/org.apache.spark.sql.sources.DataSourceRegister

@@ -1 +1,2 @@
 org.apache.spark.ml.source.libsvm.LibSVMFileFormat
+org.apache.spark.ml.source.image.ImageFileFormat


tiny nit: newline at the end - this might be caught by maven's checkstyle (#21801)

HyukjinKwon · 2018-09-04T10:09:54Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ * @note This class is public for documentation purpose. Please don't use this class directly.
+ * Rather, use the data source API as illustrated above.
+ */
+class ImageDataSource private() {}


newline at the end - technically this should be caught by scalastyle.

HyukjinKwon · 2018-09-04T10:10:21Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageFileFormat.scala

+import org.apache.spark.util.SerializableConfiguration
+
+
+private[image] class ImageFileFormatOptions(


Shall we make another file for this for consistency?

HyukjinKwon · 2018-09-04T10:12:35Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala

      "org.apache.spark.sql.execution.datasources.orc" -> nativeOrc,
      "org.apache.spark.ml.source.libsvm.DefaultSource" -> libsvm,
      "org.apache.spark.ml.source.libsvm" -> libsvm,
+      "org.apache.spark.ml.source.image.ImageFileFormat.DefaultSource" -> image,


Since this is new datasource, I think we wouldn't probably need this in backword compatibility map.

HyukjinKwon · 2018-09-04T10:15:38Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageFileFormat.scala

+      job: Job, options: Map[String, String],
+      dataSchema: StructType): OutputWriterFactory = {
+    throw new UnsupportedOperationException(
+      s"prepareWrite is not supported for image data source")


tiny nit: s can be removed

The error message is user-facing and users do not know prepareWrite. So just say "Write is not supported"

HyukjinKwon · 2018-09-04T10:16:35Z

cc @imatiach-msft

HyukjinKwon · 2018-09-04T10:18:23Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ *  - "dropImageFailures": Whether to drop the files that are not valid images from the result.
+ *
+ * @note This class is public for documentation purpose. Please don't use this class directly.
+ * Rather, use the data source API as illustrated above.


@WeichenXu123, don't we plan to make a documentation in the site?

I didn't see a section in the doc that lists all built-in data sources. It would be nice if we create a section and link it to this API doc. I think we can do it with a follow-up PR. I want to see if we can get this PR merged before branch cut:)

SparkQA · 2018-09-04T13:32:18Z

Test build #95668 has finished for PR 22328 at commit 5164d19.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

imatiach-msft · 2018-09-04T15:48:13Z

question: why were the images renamed to images/images folder from just images? Seems a bit strange to me to have the same folder name twice.

mengxr · 2018-09-04T15:44:07Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ *  - "dropImageFailures": Whether to drop the files that are not valid images from the result.
+ *
+ * @note This class is public for documentation purpose. Please don't use this class directly.
+ * Rather, use the data source API as illustrated above.


I didn't see a section in the doc that lists all built-in data sources. It would be nice if we create a section and link it to this API doc. I think we can do it with a follow-up PR. I want to see if we can get this PR merged before branch cut:)

mengxr · 2018-09-04T15:47:27Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageFileFormat.scala

+      job: Job, options: Map[String, String],
+      dataSchema: StructType): OutputWriterFactory = {
+    throw new UnsupportedOperationException(
+      s"prepareWrite is not supported for image data source")


The error message is user-facing and users do not know prepareWrite. So just say "Write is not supported"

mengxr · 2018-09-04T15:49:45Z

mllib/src/test/scala/org/apache/spark/ml/image/ImageSchemaSuite.scala

 class ImageSchemaSuite extends SparkFunSuite with MLlibTestSparkContext {
  // Single column of images named "image"
-  private lazy val imagePath = "../data/mllib/images"
+  private lazy val imagePath = "../data/mllib/images/images"


"images/images" is confusing. Call it images/origin and images/partitioned

mengxr · 2018-09-04T15:50:19Z

mllib/src/test/scala/org/apache/spark/ml/source/image/ImageFileFormatSuite.scala

+      val mode = getMode(row)
+      val bytes20 = getData(row).slice(0, 20).toList
+      filename -> Tuple2(mode, bytes20)
+    }.toMap


use Set instead of Map

mengxr · 2018-09-04T15:54:36Z

@imatiach-msft @HyukjinKwon The plan is to mark ImageSchema deprecated in 2.4 and remove it in 3.0. So loading images will be the same as loading data from other sources.

The gaps are sampling and partition controlling, which might require more testing after 2.4. It would be great if you can help. For sampling, I'm thinking of allowing data source to handle sample operations. @cloud-fan is it feasible?

imatiach-msft

added some comments

imatiach-msft · 2018-09-04T15:49:26Z

data/mllib/images/images/license.txt

@@ -0,0 +1,13 @@
+The images in the folder "kittens" are under the creative commons CC0 license, or no rights reserved:


not sure why this was added to images/images folder, also strange that it wasn't moved

update to comment above:
I don't think you need duplicate images in the PR - in the old tests you can just specify the path up to the cls/date folder. There might be a few minor changes to the tests but I think that would be a better strategy than to have duplicate images in source.

No worry. Only very few images. And keep old testcase not changed will help this PR get merged ASAP.

imatiach-msft · 2018-09-04T15:51:40Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ *  - data: BinaryType (Image bytes in OpenCV-compatible order: row-wise BGR in most cases)
+ *
+ * To use IMAGE data source, you need to set "image" as the format in `DataFrameReader` and
+ * optionally specify options, for example:


tiny nit: although it makes sense, "optionally specify options" is a bit confusing, maybe "optionally specify arguments" or just "specify options"?

The latter "options" is "datasource options", it is the widely used term.
So I prefer to change to "optionally specify the datasource options"

imatiach-msft · 2018-09-04T15:55:21Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ * optionally specify options, for example:
+ * {{{
+ *   // Scala
+ *   val df = spark.read.format("image")


this is very cool!

imatiach-msft · 2018-09-04T15:59:38Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageFileFormat.scala

+
+  override def shortName(): String = "image"
+
+  override protected def buildReader(


hmm, is there any way we could combine the two apis? I don't like having to support two different implementations. Or, what is the issue that is blocking us from combining them?

imatiach-msft · 2018-09-04T16:01:27Z

mllib/src/test/scala/org/apache/spark/ml/source/image/ImageFileFormatSuite.scala

+
+  // Images with the different number of channels
+  test("readImages pixel values test") {
+


tiny nit: remove newline (or make all tests consistent in terms of formatting)

imatiach-msft · 2018-09-04T16:05:29Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageFileFormat.scala

+      requiredSchema: StructType,
+      filters: Seq[Filter],
+      options: Map[String, String],
+      hadoopConf: Configuration): (PartitionedFile) => Iterator[InternalRow] = {


should the sampling option be ported as well? It seemed like an important option in case users didn't want to load all images.

It won't be addressed in this PR. The best way to support it is to allow data source handle sampling operation. cc @cloud-fan

sample pushdown should be supported by data source v2 in the next release, then we can migrate the image source to data source v2 at that time.

imatiach-msft · 2018-09-04T16:13:05Z

"specify numPartitions (it will be determined by datasource automatically)"
This feature was because of this bug:

https://issues.apache.org/jira/browse/SPARK-22357

Which was just fixed a week ago in spark 2.4.0.
It can be removed, even in the current API.

imatiach-msft · 2018-09-04T16:15:54Z

@mengxr "is to mark ImageSchema deprecated in 2.4 and remove it in 3.0"
confused, this is still using ImageSchema though, right? You mean to remove the top-level readImages API, but the ImageSchema will remain the same, correct?

imatiach-msft · 2018-09-04T16:23:54Z

I don't think you need duplicate images in the PR - in the old tests you can just specify the path up to the cls/date folder. There might be a few minor changes to the tests but I think that would be a better strategy than to have duplicate images in source.

mengxr · 2018-09-04T16:25:33Z

Yes, the ImageSchema implementation are used by the data source, which we cannot remove:) We are only going to mark the public APIs there as deprecated. The goal is to provide users a unified approach to load data into Spark. Users usually find ImageSchema.readImages hard to discover.

imatiach-msft · 2018-09-04T17:16:09Z

@mhamilton723 could you take a look at this PR? Mark added some performance improvements in MMLSpark that we wanted to merge in and he also added support for streaming (this was one of the PRs: https://github.com/Azure/mmlspark/pull/134/files , there were a couple more after). He also had some concerns about performance (specifically how we were storing the images as OpenCV bytes in the dataframe which he said took a lot of memory and we should use the more compressed format instead) and I recall he had a few suggestions on how we could improve it in the future. This seems like a good place to discuss how we could improve performance more.

jaceklaskowski · 2018-09-04T19:19:10Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ * `image` package implements Spark SQL data source API for loading IMAGE data as `DataFrame`.
+ * The loaded `DataFrame` has one `StructType` column: `image`.
+ * The schema of the `image` column is:
+ *  - origin: String (represent the origin of image. If loaded from file, then it is file path)


"represents" + "the image". I can see many missing as and thes in the description :(

jaceklaskowski · 2018-09-04T19:20:28Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ * {{{
+ *   // Scala
+ *   val df = spark.read.format("image")
+ *     .option("dropImageFailures", "true")


true as a boolean value, please.

option API require (k: String, v:String) parameters.

Really? What about option(key: String, value: Boolean): DataFrameReader then? There are more --> http://spark.apache.org/docs/latest/api/scala/index.html#org.apache.spark.sql.DataFrameReader

jaceklaskowski · 2018-09-04T19:22:46Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageFileFormat.scala

+
+  override def prepareWrite(
+      sparkSession: SparkSession,
+      job: Job, options: Map[String, String],


New line after job: Job

jaceklaskowski · 2018-09-04T19:25:25Z

mllib/src/test/scala/org/apache/spark/ml/source/image/ImageFileFormatSuite.scala

+    val df1 = spark.read.format("image").load(imagePath)
+    assert(df1.count === 9)
+
+    val df2 = spark.read.format("image").option("dropImageFailures", "true").load(imagePath)


true as a boolean value, please.

jaceklaskowski · 2018-09-04T19:27:47Z

mllib/src/test/scala/org/apache/spark/ml/source/image/ImageFileFormatSuite.scala

+      val filename = Paths.get(getOrigin(row)).getFileName().toString()
+      val mode = getMode(row)
+      val bytes20 = getData(row).slice(0, 20).toList
+      filename -> Tuple2(mode, bytes20)


Why is Tuple2 required here? Wouldn't (mode, bytes20) work here?

yea, (mode, bytes20) doesn't work, filename -> (mode, bytes20) will be compiled as filename.->(mode, bytes20) and -> receive 2 arguments and compile error occurs.

nit: It is useful to leave an inline comment here:)

jaceklaskowski · 2018-09-04T19:29:44Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala

    val parquet = classOf[ParquetFileFormat].getCanonicalName
    val csv = classOf[CSVFileFormat].getCanonicalName
    val libsvm = "org.apache.spark.ml.source.libsvm.LibSVMFileFormat"
+    val image = "org.apache.spark.ml.source.image.ImageFileFormat"


Why is this needed?

similar to libsvm.

We did it for libsvm for historical reasons. Since image is a new source, I don't think we have compatibility issues.

mhamilton723 · 2018-09-05T02:03:19Z

@WeichenXu123. Awesome work! I have not had a chance to go through this in depth but I did this in the originating project, MMLSpark, a while back and have been meaning to ship it up to Spark Core:

https://github.com/Azure/mmlspark/tree/master/src/io/image

We built ours on the same functionality for Binary Files and it might be wise to do the same here if you have not already. The Binary File API is a bit more general as it supports reading many different kinds of files.

https://github.com/Azure/mmlspark/tree/master/src/io/binary

mengxr · 2018-09-05T05:12:00Z

@mhamilton723 I thought about that option too. Loading general binary files is a useful feature but I don't feel it is necessary to pull it into the current scope. No matter whether the image data source has its own implementation or builds on top of the binary data source, I expect users to use

spark.read.format("image").load("...")

to read images instead of something like:

spark.read.format("binary").load("...").withColumn("image", decode($"binary"))

So we can definitely add binary file data source later and swap the implementation without changing the public interface. But we don't need to block this PR getting into 2.4, which will be cut soon.

Sounds good?

jaceklaskowski · 2018-09-05T05:17:50Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

 *
 * To use IMAGE data source, you need to set "image" as the format in `DataFrameReader` and
- * optionally specify options, for example:
+ * optionally specify the datasource options, for example:


s/datasource/data source

jaceklaskowski · 2018-09-05T05:19:08Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

 * IMAGE data source supports the following options:
 *  - "dropImageFailures": Whether to drop the files that are not valid images from the result.
 *
+ * @note This IMAGE data source does not support "write".


s/"write"/saving images to a file(s)/ ?

jaceklaskowski · 2018-09-05T05:20:38Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageOptions.scala

+
+  def this(parameters: Map[String, String]) = this(CaseInsensitiveMap(parameters))
+
+  val dropImageFailures = parameters.getOrElse("dropImageFailures", "false").toBoolean


Why false is a String not a boolean?

because parameters is Map[String, String] type.

cloud-fan · 2018-09-05T05:23:24Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ * @note This class is public for documentation purpose. Please don't use this class directly.
+ * Rather, use the data source API as illustrated above.
+ */
+class ImageDataSource private() {}


why do we need this class?

for doc.
similar to LibSVMDataSource

Is this a convention? AFAIK in the scala world we usually put document in package object.

Can we just simply remove this and make a followup for the doc in the site ..

Re: @cloud-fan The Scala package doc doesn't work for Java, which requires a different format.

Re: @HyukjinKwon It would be nice to have some doc in the site, though I didn't find the list of built-in data sources in the doc site. I think it is okay to have docs in both locations for IDE users and for people search on the web.

SparkQA · 2018-09-05T07:05:01Z

Test build #95694 has finished for PR 22328 at commit bd6178c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-09-05T07:05:01Z

Test build #95695 has finished for PR 22328 at commit 4d52754.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

jaceklaskowski · 2018-09-05T10:29:10Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ * {{{
+ *   // Scala
+ *   val df = spark.read.format("image")
+ *     .option("dropImageFailures", "true")


Really? What about option(key: String, value: Boolean): DataFrameReader then? There are more --> http://spark.apache.org/docs/latest/api/scala/index.html#org.apache.spark.sql.DataFrameReader

SparkQA · 2018-09-05T10:29:53Z

Test build #95707 has finished for PR 22328 at commit 3fffd7e.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-09-05T13:50:24Z

Test build #95715 has finished for PR 22328 at commit 3c8863c.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mengxr · 2018-09-05T15:34:05Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+package org.apache.spark.ml.source.image
+
+/**
+ * `image` package implements Spark SQL data source API for loading IMAGE data as `DataFrame`.


"IMAGE" doesn't need to be all uppercase. Just say "loading images".

mengxr · 2018-09-05T15:34:32Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ * The loaded `DataFrame` has one `StructType` column: `image`.
+ * The schema of the `image` column is:
+ *  - origin: String (represents the origin of the image.
+ *                    If loaded from files, then it is the file path)


does it always load from files?

mengxr · 2018-09-05T15:35:42Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ *  - mode: Int (OpenCV-compatible type)
+ *  - data: BinaryType (Image bytes in OpenCV-compatible order: row-wise BGR in most cases)
+ *
+ * To use IMAGE data source, you need to set "image" as the format in `DataFrameReader` and


ditto on "IMAGE"

mengxr · 2018-09-05T15:37:21Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageDataSource.scala

+ * }}}
+ *
+ * IMAGE data source supports the following options:
+ *  - "dropImageFailures": Whether to drop the files that are not valid images from the result.


How about changing dropImageFailures to dropInvalid?

mengxr · 2018-09-05T15:39:07Z

mllib/src/main/scala/org/apache/spark/ml/source/image/ImageOptions.scala

+
+  def this(parameters: Map[String, String]) = this(CaseInsensitiveMap(parameters))
+
+  val dropImageFailures = parameters.getOrElse("dropImageFailures", "false").toBoolean


Should add ScalaDoc.

mengxr · 2018-09-05T15:40:22Z

mllib/src/test/scala/org/apache/spark/ml/source/image/ImageFileFormatSuite.scala

+      val filename = Paths.get(getOrigin(row)).getFileName().toString()
+      val mode = getMode(row)
+      val bytes20 = getData(row).slice(0, 20).toList
+      filename -> Tuple2(mode, bytes20)


nit: It is useful to leave an inline comment here:)

mengxr · 2018-09-05T16:05:41Z

LGTM pending tests.

SparkQA · 2018-09-05T16:33:28Z

Test build #95724 has finished for PR 22328 at commit 218ce4c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mengxr · 2018-09-05T17:20:10Z

test this please

mengxr · 2018-09-05T17:21:23Z

The image data source tests passed but JVM crashed at the end. Triggered another test.

SparkQA · 2018-09-05T18:36:53Z

Test build #95727 has finished for PR 22328 at commit 218ce4c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mengxr · 2018-09-05T19:16:28Z

Merged into master. Thanks @WeichenXu123 for the implementation and everyone for the review! I created the following JIRAs as follow-ups:

deprecate ImageSchema: https://issues.apache.org/jira/browse/SPARK-25345
list built-in data sources in doc site: https://issues.apache.org/jira/browse/SPARK-25346
doc for image data source: https://issues.apache.org/jira/browse/SPARK-25347
data source for binary files: https://issues.apache.org/jira/browse/SPARK-25348
support sample pushdown: https://issues.apache.org/jira/browse/SPARK-25349

init pr

5b5aee6

fix scala style

5164d19

HyukjinKwon reviewed Sep 4, 2018

View reviewed changes

mengxr requested changes Sep 4, 2018

View reviewed changes

imatiach-msft suggested changes Sep 4, 2018

View reviewed changes

jaceklaskowski suggested changes Sep 4, 2018

View reviewed changes

address comments

bd6178c

jaceklaskowski suggested changes Sep 5, 2018

View reviewed changes

cloud-fan reviewed Sep 5, 2018

View reviewed changes

address comments

4d52754

nit update

3fffd7e

jaceklaskowski suggested changes Sep 5, 2018

View reviewed changes

cloud-fan approved these changes Sep 5, 2018

View reviewed changes

more nit updates

3c8863c

mengxr requested changes Sep 5, 2018

View reviewed changes

address comments

218ce4c

asfgit closed this in 9254492 Sep 5, 2018

WeichenXu123 deleted the image_datasource branch September 6, 2018 01:40

		@@ -1 +1,2 @@
		org.apache.spark.ml.source.libsvm.LibSVMFileFormat
		org.apache.spark.ml.source.image.ImageFileFormat

		import org.apache.spark.util.SerializableConfiguration


		private[image] class ImageFileFormatOptions(

		@@ -0,0 +1,13 @@
		The images in the folder "kittens" are under the creative commons CC0 license, or no rights reserved:


		override def shortName(): String = "image"

		override protected def buildReader(


		// Images with the different number of channels
		test("readImages pixel values test") {


		def this(parameters: Map[String, String]) = this(CaseInsensitiveMap(parameters))

		val dropImageFailures = parameters.getOrElse("dropImageFailures", "false").toBoolean

[SPARK-22666][ML][SQL] Spark datasource for image format #22328

[SPARK-22666][ML][SQL] Spark datasource for image format #22328

Uh oh!

Conversation

WeichenXu123 commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Benchmark

Uh oh!

SparkQA commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 4, 2018

Uh oh!

imatiach-msft commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengxr commented Sep 4, 2018

Uh oh!

imatiach-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imatiach-msft commented Sep 4, 2018

Uh oh!

imatiach-msft commented Sep 4, 2018

Uh oh!

imatiach-msft commented Sep 4, 2018

Uh oh!

mengxr commented Sep 4, 2018

Uh oh!

imatiach-msft commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

WeichenXu123 commented Sep 4, 2018 •

edited

Loading