Estimate partitions number basing on GeoTiff segments by pomadchin · Pull Request #2296 · locationtech/geotrellis

pomadchin · 2017-07-24T13:14:38Z

Poor performance fix
Tests (new & fix old)
Better repartition in border cases (size / windows size in stripped / tiles cases)
Rethink multiband fromSegments function, see d952665
Rethink a singleband fromSegments function

The aim of two last points is to reduce memory usage

…sired partition size in bytes. Signed-off-by: Grisha Pomadchin <[email protected]>

Signed-off-by: Grisha Pomadchin <[email protected]>

echeipesh · 2017-07-25T19:09:27Z

+  private def _subsetBandsFromSegments(
+    bandSequence: Seq[Int],
+    segmentIds: Traversable[Int],
+    deinterleaveBitSegment: (GeoTiffSegment, Int, Int, Int, Traversable[Int]) => Array[Array[Byte]],


Thats a rough signature. Is there any way to sugar it up? Its used a couple of times, maybe type alias where we can hang a comment?

echeipesh · 2017-07-25T19:13:15Z

      (bytes, bandCount, bytesPerSample, _) => GeoTiffSegment.deinterleave(bytes, bandCount, bytesPerSample)
    ).toVector

+  def bandsFromSegments(segmentIds: Traversable[Int]): Vector[Tile] =


Should be private as not to add to API ?

echeipesh · 2017-07-25T19:35:09Z

    }
-  }

+    def readSegments(ids: Traversable[Int], info: GeoTiffReader.GeoTiffInfo, options: Options) = {


Needs return type

echeipesh · 2017-07-25T19:57:34Z

+
+import scala.collection.mutable
+
+case class S3GeoTiffInfoReader(


By the way this is used it looks like it could be decomposed into a companion object with some methods on it, that would probably be safer than a case class that potentially carries client around.

…iant to read by segments, code refactor. Signed-off-by: Grisha Pomadchin <[email protected]>

pomadchin · 2017-07-26T22:12:02Z

+  /** Read segments iterator, each segment into a single R */
+  def readSegmentsIterator(gbs: Array[GridBounds], info: GeoTiffReader.GeoTiffInfo, options: O): Iterator[R]
+  /** Reads all segments into one R */
+  def readSegments(gbs: Array[GridBounds], info: GeoTiffReader.GeoTiffInfo, options: O): Option[R]


Two strategies to read segments, which is better for our use cases?

pomadchin · 2017-07-26T22:27:21Z

+    * each segment can only be in a single partition.
+    * */
+  def segmentsByPartitionBytes(partitionBytes: Long = Long.MaxValue, maxTileSize: Option[Int] = None)
+                              (implicit sc: SparkContext): RDD[((String, GeoTiffInfo), Array[GridBounds])] = {


We can still operate with segment refs

Signed-off-by: Grisha Pomadchin <[email protected]>

…t of GeoTiffInfoReader function private Signed-off-by: Grisha Pomadchin <[email protected]>

echeipesh · 2017-07-28T15:47:59Z

    * @param delimiter      Delimiter to use for S3 objet listings. See
    * @param getS3Client    A function to instantiate an S3Client. Must be serializable.
+    * @param persistLevel   A spark persist sotrage level, MEMORY_ONLY by default (similar to RDD.cache())
+    * @param bySegments     Minimize segments reads, read input data by segments.


Semver doesn't give a lot of room here, can't change the Options object.

Lets assume that persistLevel is MEMORY_ONLY, if there is not enough memory to read the tiff headers, its probably a lost cause anyway. Also re-reading them twice if memory cache is purged is good soft failover.

Lets keep the maxWindowSize=None as default, that will be the flag for using the segment wise read. Setting it to some value will be the flag for reading exactly those window chunks, as before.

This means that we will never read full GeoTiff again unless its smaller than the maxWindowSize value. It seems that that GeoTiffs small enough to be appropriate for RDD record basically don't get written, so this is good default

So you think for by segment reads we don't need that window option by default? (at least in 1.2); But that can be a good feature to try to pack segments into windows we want.

It seems like the actual size of the window is not important there because the partition size will control it. If there are too few windows in partitions, you are packing more. You never splice segments together, so the effect of the window size is just to provide logic to keep the segments spatially related. So I would assume that it can just be hard coded to 1024 for segment reads.

echeipesh · 2017-07-28T15:50:08Z

+          else segments
+
+        val result = repartition.flatMap { case ((key, md), segmentIndices) =>
+          rr.readWindows(segmentIndices, md, options).map { case (k, v) =>


segmentIndices is stale variable name, I guess this is segmentBounds now ?

echeipesh · 2017-07-28T15:52:58Z

+
+import scala.collection.mutable
+
+trait GeoTiffInfoReader extends LazyLogging {


Why not just make the the private [geotrellis] trait ?

pomadchin force-pushed the feature/s3tiff-segments branch from 51a4abe to 15baaef Compare July 24, 2017 13:15

pomadchin changed the title ~~[WIP] Estimate partitions number basing on GeoTiff segments~~ Estimate partitions number basing on GeoTiff segments Jul 25, 2017

pomadchin force-pushed the feature/s3tiff-segments branch from 3e43b8f to 42e05fa Compare July 25, 2017 13:04

pomadchin requested a review from echeipesh July 25, 2017 13:58

pomadchin changed the title ~~Estimate partitions number basing on GeoTiff segments~~ [WIP] Estimate partitions number basing on GeoTiff segments Jul 25, 2017

pomadchin force-pushed the feature/s3tiff-segments branch from 85a3a7e to 8d991e3 Compare July 25, 2017 15:37

pomadchin added 8 commits July 25, 2017 18:37

Added S3 metadata reader to estimate partitions number basing on a de…

18c3176

…sired partition size in bytes. Signed-off-by: Grisha Pomadchin <[email protected]>

Add segment splits

995f4ee

Add segments reads

1ed541d

Signed-off-by: Grisha Pomadchin <[email protected]>

Fix singleband tile regression case

3ce61c6

Fix by segment reads

8aa8614

TODO commit

a9c9227

Make multiband case better

79c522d

Signed-off-by: Grisha Pomadchin <[email protected]>

Improve repartition strategy.

d952665

Signed-off-by: Grisha Pomadchin <[email protected]>

pomadchin force-pushed the feature/s3tiff-segments branch from 8d991e3 to d952665 Compare July 25, 2017 15:43

echeipesh reviewed Jul 25, 2017

View reviewed changes

pomadchin added 3 commits July 26, 2017 14:04

Optimise fromSegments funcs

a1ee681

Use crop instead of per segment reads

a8c8464

Use crop function istead of bySegments, added Iterator and Option var…

a537d30

…iant to read by segments, code refactor. Signed-off-by: Grisha Pomadchin <[email protected]>

pomadchin commented Jul 26, 2017

View reviewed changes

pomadchin changed the title ~~[WIP] Estimate partitions number basing on GeoTiff segments~~ Estimate partitions number basing on GeoTiff segments Jul 26, 2017

pomadchin commented Jul 26, 2017

View reviewed changes

pomadchin added 2 commits July 28, 2017 17:28

Fix tests to use fromSegmentsIterator, looks like a cfor bug / feature

0f68fed

Signed-off-by: Grisha Pomadchin <[email protected]>

Remove useless method, rename fromSegments into fromWindows, make mos…

cbd9b60

…t of GeoTiffInfoReader function private Signed-off-by: Grisha Pomadchin <[email protected]>

echeipesh requested changes Jul 28, 2017

View reviewed changes

echeipesh reviewed Jul 28, 2017

View reviewed changes

Satisfy Eugene comments

9bedc00

pomadchin force-pushed the feature/s3tiff-segments branch from 59504a4 to 9bedc00 Compare July 31, 2017 17:33

Fix tests

70e4634

echeipesh added this to the 1.2 milestone Aug 1, 2017

echeipesh approved these changes Aug 1, 2017

View reviewed changes

echeipesh merged commit d506901 into locationtech:master Aug 1, 2017

This was referenced Aug 2, 2017

Estimate partitions number basing on a desired partition size #2289

Closed

Unexpected Behavior When Reading GeoTiffs From S3 #2307

Closed

echeipesh mentioned this pull request Aug 31, 2017

Partition GeoTiff reading by GeoTiff segment layout #2173

Closed

2 tasks done

Navigation Menu

Repository navigation

Estimate partitions number basing on GeoTiff segments - #2296

Estimate partitions number basing on GeoTiff segments#2296
echeipesh merged 15 commits into
locationtech:masterfrom
pomadchin:feature/s3tiff-segments

pomadchin commented Jul 24, 2017 •

edited

Loading

Uh oh!

echeipesh Jul 25, 2017

Uh oh!

echeipesh Jul 25, 2017

Uh oh!

echeipesh Jul 25, 2017

Uh oh!

echeipesh Jul 25, 2017

Uh oh!

pomadchin Jul 26, 2017

Uh oh!

pomadchin Jul 26, 2017

Uh oh!

echeipesh Jul 28, 2017

Uh oh!

pomadchin Jul 29, 2017

Uh oh!

echeipesh Jul 31, 2017

Uh oh!

echeipesh Jul 28, 2017 •

edited

Loading

Uh oh!

echeipesh Jul 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		import scala.collection.mutable

		case class S3GeoTiffInfoReader(


		import scala.collection.mutable

		trait GeoTiffInfoReader extends LazyLogging {

Conversation

pomadchin commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

echeipesh Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pomadchin commented Jul 24, 2017 •

edited

Loading

echeipesh Jul 28, 2017 •

edited

Loading