Support for named transforms. #357

andrewsmartin · 2016-12-12T20:58:24Z

After some discussion in #354 we decided for now this would be the best approach. This is ready for review, but there are a couple other things we will need to address before everything is fully supported

Scio defines several composite transforms (for example in DoubleSCollectionFunctions) that use the current naming strategy (CallSites.getCurrent), so even if the user gives a name to the outer transform, the inner transform is still named non-deterministically and would be a problem for updating. We will need to go through the Scio API and identify all the places where we internally use composite transforms, and give them all an appropriate name similar to what the Dataflow SDK does for its composite transforms.
Documentation and more rigorous testing.

I propose we get this out which will solve the problem for the majority of transforms, and open another issue to address the remaining work in the above items.

nevillelyh · 2016-12-12T21:17:08Z

I'm OK with the user facing API, but feel like that we should re-work the internals a bit, especially with things carried over in d0e1590.

I'd remove the optional argument CallSites.getCurrent, and replace it with a trait NameProvider in PCollectionWrapper which either returns current stack or static string depending on the context. E.g.

trait NameProvider {
  def apply(): String
}
object CallSitesNameProvider extends NameProvider {
  @override def apply(): String = CallSites.getCurrent
}
class StaticNameProvided(private val name: String) extends NameProvider {
  @override def apply(): String = name
}

This way we can undo some of the optional args (which are always tricky) in d0e1590

nevillelyh · 2016-12-12T21:18:09Z

Basically I'm suggesting that we revert d0e1590 and use the provider approach which is cleaner and more extendable.

andrewsmartin · 2016-12-12T21:18:35Z

+1, do as part of this PR?

nevillelyh · 2016-12-12T21:19:58Z

scio-core/src/main/scala/com/spotify/scio/package.scala

@@ -78,6 +78,12 @@ package object scio {
      Await.result(self.flatMap(identity), atMost)
  }

+  /** Set the name of the next transform. */
+  implicit class NamedTfSCollection[T: ClassTag](val wrapped: SCollection[T]) {
+    def withName(tfName: String): SCollection[T] =


This should fail if wrapped is already a NamedTfSCollection. Also I'd name this NamedSCollection.

True. Easiest way is probably just to have wrapped be SCollectionImpl instead right?

Actually that won't work because user only sees SCollection[T]

Probably true, you might not even need a NamedSCollectionImpl, just reflect on self.nameProvider should be sufficient.

I mean you might not need a implicit class, have def .withName on PCollectionWrapper would be the best/cleanest, if we can get rid of CallSites.getCurrent everywhere else and replace them with this.nameProvider.get().

nevillelyh · 2016-12-12T21:21:10Z

Sure. You can do multi-commits PRs. It's clearer if we keep a revert commit (git revert $hash).

andrewsmartin · 2016-12-13T19:47:03Z

@nevillelyh @ravwojdyla This solution works for all SCollection variants, any thoughts?

andrewsmartin · 2016-12-13T20:04:58Z

Should note that this still doesn't prevent calling .withName() twice in a row, would like to get some opinions on the best way to approach. IMO I don't think it's something we need to care about - if a user does it twice before applying a transform, the least-astonishing thing would be to just overwrite the var again.

codecov-io · 2016-12-13T21:41:54Z

Current coverage is 81.65% (diff: 100%)

Merging #357 into master will decrease coverage by 0.55%

@@             master       #357   diff @@
==========================================
  Files            66         67     +1   
  Lines          2540       2627    +87   
  Methods        2267       2327    +60   
  Messages          0          0          
  Branches        273        300    +27   
==========================================
+ Hits           2088       2145    +57   
- Misses          452        482    +30   
  Partials          0          0

Powered by Codecov. Last update f8d6186...49737c0

nevillelyh · 2016-12-14T10:49:07Z

scio-core/src/main/scala/com/spotify/scio/values/TransformNameable.scala

+import com.spotify.scio.util.CallSites
+
+trait TransformNameable[T <: TransformNameable[T]] {
+  this: T =>


I think the convention is to keep this: T => on the previous line.

nevillelyh · 2016-12-14T10:50:12Z

scio-core/src/main/scala/com/spotify/scio/values/TransformNameable.scala

+  def tfName: String = nameProvider.name
+
+  def withName(name: String): T = {
+    nameProvider = new ConstNameProvider(name)


add an require() here against withName().withName() ?

nevillelyh · 2016-12-14T10:54:13Z

Also do a git grep CallSites in the code base?
There're also some join and multi-join cases.
MultiJoin is an object right now but shouldn't be too hard to add withName so we can have MultiJoin.withName("x").join(a, b, c, d)?

andrewsmartin · 2016-12-14T18:58:31Z

@nevillelyh just pushed two more commits to address your comments.

ravwojdyla · 2016-12-16T00:40:09Z

👍

Don't leak variable transform name to subclasses. Also fix whitespace and make test names consistent. Add TransformNameProvider so CallSites.getCurrent can be deferred to the proper call site. Fix test.

Also give explicit names to internal transforms in DoubleSCollectionFunctions

nevillelyh · 2017-01-17T19:07:38Z

scio-core/src/main/scala/com/spotify/scio/accumulators/package.scala

@@ -79,8 +79,8 @@ package object accumulators {

    /** Count positive and negative results in a filter with automatically created accumulators. */
    def accumulateCountFilter(f: T => Boolean): SCollection[T] = {
-      val accPos = self.context.sumAccumulator[Long]("Positive#" + CallSites.getCurrentName)
-      val accNeg = self.context.sumAccumulator[Long]("Negative#" + CallSites.getCurrentName)
+      val accPos = self.context.sumAccumulator[Long]("Positive#" + self.tfName)


These 2 are not PTransform names but accumulator identifiers. We should keep the original so that they're distinguishable.

nevillelyh · 2017-01-17T19:08:08Z

scio-core/src/main/scala/com/spotify/scio/accumulators/package.scala

@@ -60,7 +60,7 @@ package object accumulators {

    /** Count elements with an automatically created accumulator. */
    def accumulateCount: SCollection[T] =
-      self.accumulateCount(self.context.sumAccumulator[Long](CallSites.getCurrentName))


This is an accumulator name. Keep the original.

nevillelyh · 2017-01-17T19:11:45Z

scio-core/src/main/scala/com/spotify/scio/values/DoubleSCollectionFunctions.scala

@@ -39,22 +39,26 @@ class DoubleSCollectionFunctions(self: SCollection[Double]) {
  // def sum: SCollection[Double] = this.stats().map(_.sum)

  /** Compute the standard deviation of this SCollection's elements. */
-  def stdev: SCollection[Double] = self.transform(_.stats.map(_.stdev))
+  def stdev: SCollection[Double] = self.transform(_.stats.withName("StdDev").map(_.stdev))


Let's leave transforms in this file alone for now? There're too many of them and it's impossible to catch all. Like there're a few in histogram() that we didn't cover?

Plus I doubt these are super useful in streaming. Better to catch them later as they pop up.

I did this up front so that we can have a test for naming transforms on DoubleSCollectionFunctions. Since they're composite transforms I wanted to make the internal name deterministic to make an assertion. https://github.com/spotify/scio/blob/playground/scio-test/src/test/scala/com/spotify/scio/values/NamedTransformTest.scala#L45

Maybe instead I can remove these, and just assert the transform name starts with the given name and ignore the inner transform name?

Agree we can just assert the part before /. Leave the inner ones alone for now and fix when it becomes a problem.

nevillelyh · 2017-01-17T19:13:50Z

scio-core/src/main/scala/com/spotify/scio/values/SCollection.scala

@@ -60,8 +60,8 @@ object SCollection {
  def unionAll[T: ClassTag](scs: Iterable[SCollection[T]]): SCollection[T] = {
    val o = PCollectionList
      .of(scs.map(_.internal).asJava)
-      .apply(CallSites.getCurrent, Flatten.pCollections())
-    new SCollectionImpl(o, scs.head.context)
+      .apply("FlattenList", Flatten.pCollections())


Use UnionAll to be more precise?

nevillelyh · 2017-01-17T19:14:39Z

scio-core/src/main/scala/com/spotify/scio/values/SCollection.scala

@@ -163,7 +163,7 @@ sealed trait SCollection[T] extends PCollectionWrapper[T] {
  def union(that: SCollection[T]): SCollection[T] = {
    val o = PCollectionList
      .of(internal).and(that.internal)
-      .apply(CallSites.getCurrent, Flatten.pCollections())
+      .apply("FlattenList", Flatten.pCollections())


Can use this.tfName here? More consistent with other transforms.

nevillelyh · 2017-01-17T19:27:48Z

scio-core/src/main/scala/com/spotify/scio/values/TransformNameable.scala

+trait TransformNameable {
+  private var nameProvider: TransformNameProvider = CallSiteNameProvider
+
+  def tfName: String = {


make this private[values]?

ScioContext also needs this, will make it private[scio]

nevillelyh · 2017-01-17T19:29:29Z

scio-test/src/test/scala/com/spotify/scio/values/NamedTransformTest.scala

+        .withName("MyTransform").map(_ * 3)
+      val p3 = p1
+        .withName("MyTransform").map(_ * 4)
+      assertTransformName(p1, "MyTransform")


Can't find the incremental logic in this PR, where are they from? Is it better to fail duplicate names instead?

This logic is built-in to dataflow, I included a test for it because an earlier version of this PR actually broke in this scenario.

* Add ability to name transforms in all SCollection variants. Don't leak variable transform name to subclasses. Also fix whitespace and make test names consistent. Add TransformNameProvider so CallSites.getCurrent can be deferred to the proper call site. Fix test. * Remove all occurrences of CallSites.getCurrent Also give explicit names to internal transforms in DoubleSCollectionFunctions * Prevent successive calls to withName * Always reset NameProvider after tfName requested. * No need to use F-bounded type for TransformNameable. * Accumulator names should still use CallSites.getCurrentName * Fix transform names for union and unionAll * Ignore internal transform names in DoubleSCollectionFunctions for now. * Make tfName scio-private * Better test helper function names. * Fix formatting Conflicts: scio-core/src/main/scala/com/spotify/scio/util/MultiJoin.scala scio-core/src/main/scala/com/spotify/scio/values/PCollectionWrapper.scala scio-core/src/main/scala/com/spotify/scio/values/SCollection.scala

andrewsmartin force-pushed the playground branch 2 times, most recently from 3309992 to ce3fe78 Compare December 12, 2016 21:00

This was referenced Dec 12, 2016

Idea 1 for named transforms #354

Closed

Give deterministic names to internal transforms. #358

Closed

nevillelyh reviewed Dec 12, 2016

View reviewed changes

nevillelyh force-pushed the master branch from 812d03f to 46424c4 Compare December 13, 2016 13:26

andrewsmartin force-pushed the playground branch from ce3fe78 to dd19b5c Compare December 13, 2016 19:40

andrewsmartin force-pushed the playground branch from aad2628 to 28ad371 Compare December 13, 2016 19:57

andrewsmartin force-pushed the playground branch from 09517f1 to 6e1560d Compare December 13, 2016 21:32

andrewsmartin force-pushed the playground branch 2 times, most recently from 578dd6b to f62fefc Compare December 13, 2016 23:52

nevillelyh reviewed Dec 14, 2016

View reviewed changes

andrewsmartin force-pushed the playground branch from 801ba10 to 1822fe6 Compare December 14, 2016 19:14

nevillelyh force-pushed the master branch from 0abe829 to f8d6186 Compare January 10, 2017 15:18

andrewsmartin added 5 commits January 17, 2017 11:27

Add ability to name transforms in all SCollection variants.

de8c99a

Don't leak variable transform name to subclasses. Also fix whitespace and make test names consistent. Add TransformNameProvider so CallSites.getCurrent can be deferred to the proper call site. Fix test.

Remove all occurrences of CallSites.getCurrent

6289fd8

Also give explicit names to internal transforms in DoubleSCollectionFunctions

Prevent successive calls to withName

940b48c

Always reset NameProvider after tfName requested.

64a7247

No need to use F-bounded type for TransformNameable.

98c2539

andrewsmartin force-pushed the playground branch from b30261f to 98c2539 Compare January 17, 2017 16:27

nevillelyh reviewed Jan 17, 2017

View reviewed changes

Accumulator names should still use CallSites.getCurrentName

407b4e6

nevillelyh reviewed Jan 17, 2017

View reviewed changes

andrewsmartin added 5 commits January 17, 2017 14:37

Fix transform names for union and unionAll

fa29b25

Ignore internal transform names in DoubleSCollectionFunctions for now.

3139408

Make tfName scio-private

9d8bde8

Better test helper function names.

49737c0

Fix formatting

0d2fa9e

andrewsmartin merged commit 535543e into master Jan 17, 2017

andrewsmartin deleted the playground branch January 17, 2017 20:38

echeran mentioned this pull request Feb 23, 2017

MultiJoin transform warns of no "stable unique name" #466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for named transforms. #357

Support for named transforms. #357

andrewsmartin commented Dec 12, 2016

nevillelyh commented Dec 12, 2016

nevillelyh commented Dec 12, 2016

andrewsmartin commented Dec 12, 2016

nevillelyh Dec 12, 2016

andrewsmartin Dec 12, 2016

andrewsmartin Dec 12, 2016

nevillelyh Dec 12, 2016

nevillelyh Dec 12, 2016

nevillelyh commented Dec 12, 2016

andrewsmartin commented Dec 13, 2016

andrewsmartin commented Dec 13, 2016 •

edited

Loading

codecov-io commented Dec 13, 2016 •

edited

Loading

nevillelyh Dec 14, 2016

nevillelyh Dec 14, 2016

nevillelyh commented Dec 14, 2016

andrewsmartin commented Dec 14, 2016

ravwojdyla commented Dec 16, 2016

nevillelyh Jan 17, 2017

nevillelyh Jan 17, 2017

nevillelyh Jan 17, 2017

andrewsmartin Jan 17, 2017

nevillelyh Jan 17, 2017 •

edited

Loading

nevillelyh Jan 17, 2017

nevillelyh Jan 17, 2017

nevillelyh Jan 17, 2017

andrewsmartin Jan 17, 2017

nevillelyh Jan 17, 2017

andrewsmartin Jan 17, 2017

nevillelyh Jan 17, 2017

Support for named transforms. #357

Support for named transforms. #357

Conversation

andrewsmartin commented Dec 12, 2016

nevillelyh commented Dec 12, 2016

nevillelyh commented Dec 12, 2016

andrewsmartin commented Dec 12, 2016

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

nevillelyh commented Dec 12, 2016

andrewsmartin commented Dec 13, 2016

andrewsmartin commented Dec 13, 2016 • edited Loading

codecov-io commented Dec 13, 2016 • edited Loading

Current coverage is 81.65% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevillelyh commented Dec 14, 2016

andrewsmartin commented Dec 14, 2016

ravwojdyla commented Dec 16, 2016

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

nevillelyh Jan 17, 2017 • edited Loading

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

andrewsmartin commented Dec 13, 2016 •

edited

Loading

codecov-io commented Dec 13, 2016 •

edited

Loading

nevillelyh Jan 17, 2017 •

edited

Loading