Add cross-build for Scala 2.12 #600

sritchie · 2016-12-03T03:41:56Z

Various plugin and SBT upgrades. I managed to get rid of all deprecation warnings in the build.sbt, which is a nice start to this new era.

The three issues we found were

a legit bug with transient lazy val serialization on scala 2.12.0 - this is fixed by 2.12.1.
a bug with Interval (Either gained a contains method, screwing up our implicit conversion trick)
a bug with a generator in one of the tests, fixed by using Gen.nonEmptyList

We also did some work to remove the Ordering instance from the Interval trait, since we're going binary-incompatible on this release.

The only library excluded is algebird-spark, since spark isn't on 2.12 yet. Turns out they're waiting on our chill release!

sritchie · 2016-12-03T03:51:20Z

Whelp, looks like just killing those overrides breaks QTree. Leaving this one to the pros!

johnynek · 2016-12-03T17:23:38Z

@ianoc we are willing to break binary compatibility here so maybe we can clean up.

As I recall, you made this change to make the hashCode lazy, is that right?

Did you look up some alternate syntax that would work here? We could always just manually implement a hashCode for these types (also not using lazy is probably worth it, and just checking if the hashCode is 0 or something and recomputing if it is).

ianoc · 2016-12-03T17:32:39Z

Yeah i made the change, it has a pretty big performance impact. I don't really care how it gets done, I'd probably keep it using lazy unless it makes a visible change to a benchmark, just that its obvious what it does to anyone driving past and doesn't seem necessary for the fix.

Upstream just up and deleted these functions iirc, since the code generation in case classes doesn't call them anymore, i believe they inject the hash code function now directly into the generated class.

the method in 2.11 is just
scala.util.hashing.MurmurHash3.productHash(this)

which still exists in 2.12, so the minimal change is to just use
https://github.com/scala/scala/blob/2.12.x/src/library/scala/util/hashing/MurmurHash3.scala#L211
directly?

sritchie · 2016-12-03T19:54:14Z

This excludes algebird-spark, since spark isn't ready for 2.12 primetime yet. Also fixes deprecation warnings from the SBT upgrade (there's still one left that I'm not sure how to fix)

sritchie · 2016-12-03T20:00:45Z

Legit failures now:

[info] IntervalLaws:
[info] - [x, x + 1) contains x *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (CheckProperties.scala:12)
[info]     Falsified after 0 successful property evaluations.
[info]     Location: (CheckProperties.scala:12)
[info]     Occurred when passed generated values (
[info]       arg0 = 0 // 32 shrinks
[info]     )
[info] - (x, x + 1] contains x + 1 *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (CheckProperties.scala:12)
[info]     Falsified after 0 successful property evaluations.
[info]     Location: (CheckProperties.scala:12)
[info]     Occurred when passed generated values (
[info]       arg0 = 0 // 32 shrinks
[info]     )

And this serialization error:

[info] - should not serialize BFInstance *** FAILED ***
[info]   "...ebird.BF"Ԉ��Y�X�xp[0sr,com.googlecode.javaewah.EWAHCompressedBitma��f�5I8xpw�0�������xpsr�com.twitter.algebird.BFHash��PP\l����I	numHashesI�sizeI�widthxp��0]" did not equal "...ebird.BF"Ԉ��Y�X�xp[�0sr,com.googlecode.javaewah.EWAHCompressedBitma��f�5I8xpw�0�������xsr�com.twitter.algebird.BFInstance��	������bitmap$0I�numBitsI	numHashesI�widthL�bitst#Lscala/collection/immutable/BitSet;L�hashesq~�L�sizet"Lcom/twitter/algebird/Approximate;xq~����0sr)scala.collection.immutable.BitSet$BitSet1�]��FD����J�elemsxr!scala.collection.immutable.BitSet�\�2�����xp�����sr�com.twitter.algebird.BFHash��PP\l����I	numHashesI�sizeI�widthxp��0sr com.twitter.algebird.Approximate����8d:v��D�probWithinBoundsestimatet�Ljava/lang/Object;L�maxq~�L�minq~�L�numerict�Lscala/math/Numeric;xpsr�java.lang.Long;��̏#���J�valuexr�java.lang.Number���������xsq~�	sq~��sr"scala.math.Numeric$LongIsIntegral$��wԗ����xpq~�]" (BloomFilterTest.scala:266)

sritchie · 2016-12-03T20:12:45Z

Here's the bug for interval, it's legit:

scala> Interval.leftClosedRightOpen(0, 1).contains(0)
<console>:15: warning: a type was inferred to be `Any`; this may indicate a programming error.
       Interval.leftClosedRightOpen(0, 1).contains(0)
res8: Boolean = false

sritchie · 2016-12-03T20:30:01Z

This bug too for 2.12

[info] ! CountMinSketch.CMS works for small lists: Exception raised on property evaluation.
[info] > Exception: java.lang.IllegalArgumentException: oneOf called on empty collection
[info] org.scalacheck.Gen$.oneOf(Gen.scala:471)
[info] com.twitter.algebird.CmsFrequencyProperty.inputGenerator(CountMinSketchTest.scala:320)
[info] com.twitter.algebird.CmsFrequencyProperty.inputGenerator(CountMinSketchTest.scala:312)
[info] com.twitter.algebird.ApproximateProperty$.$anonfun$successesAndProbabilities$1(ApproximateProperty.scala:39)
[info] com.twitter.algebird.ApproximateProperty$.successesAndProbabilities(ApproximateProperty.scala:37)
[info] com.twitter.algebird.ApproximateProperty$.$anonfun$toProp$1(ApproximateProperty.scala:58)
[info] com.twitter.algebird.ApproximateProperty$$$Lambda$5713/1306151911.apply(null:-1)
[info] org.scalacheck.Prop$.$anonfun$apply$1(Prop.scala:292)
[info] com.twitter.algebird.ApproximateProperty$$$Lambda$5733/1466457919.apply(null:-1)
[info] org.scalacheck.PropFromFun.apply(Prop.scala:22)
[info] org.scalacheck.Prop$.$anonfun$delay$1(Prop.scala:461)
[info] org.scalacheck.Prop$$$Lambda$2719/509484219.apply(null:-1)
[info] org.scalacheck.Prop$.$anonfun$apply$1(Prop.scala:292)
[info] org.scalacheck.Prop$$$Lambda$2720/724561827.apply(null:-1)
[info] org.scalacheck.PropFromFun.apply(Prop.scala:22)
[info] org.scalacheck.Test$.workerFun$1(Test.scala:294)
[info] org.scalacheck.Test$.$anonfun$check$1(Test.scala:323)
[info] org.scalacheck.Test$.$anonfun$check$1$adapted(Test.scala:323)
[info] org.scalacheck.Test$$$Lambda$3523/1495267299.apply(null:-1)
[info] org.scalacheck.Platform$.runWorkers(Platform.scala:40)
[info] org.scalacheck.Test$.check(Test.scala:323)
[info] org.scalacheck.ScalaCheckRunner$$anon$2.$anonfun$execute$5(ScalaCheckFramework.scala:102)
[info] org.scalacheck.ScalaCheckRunner$$anon$2.$anonfun$execute$5$adapted(ScalaCheckFramework.scala:100)
[info] org.scalacheck.ScalaCheckRunner$$anon$2$$Lambda$5703/42367799.apply(null:-1)
[info] scala.collection.TraversableLike$WithFilter.$anonfun$foreach$1(TraversableLike.scala:742)
[info] scala.collection.TraversableLike$WithFilter$$Lambda$245/1914758367.apply(null:-1)
[info] scala.collection.immutable.List.foreach(List.scala:378)
[info] scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:44)

johnynek · 2016-12-03T20:32:53Z

The Any being inferred looks like an issue to me.

The second issue is probably a change due to how scalacheck changed, I would think.

johnynek · 2016-12-03T20:34:14Z

The serialization test seems to say (didn't look in detail yet) that it should not serialize but does? Maybe that is just a weird test or testing some detail that changes with java 8.

sritchie · 2016-12-03T20:38:31Z

yeah, looks like this change for scalacheck typelevel/scalacheck@a2100f6

sritchie · 2016-12-03T20:45:20Z

calling it quits for now. The build works, so we can iterate on the three issues:

"should not serialize" bug
scalacheck getting an empty list for oneOf, which was invalid anyway
Any inference in interval

Otherwise all the build BS is cleaned up!

isnotinvain · 2016-12-06T02:20:43Z

Ah yeah we ran into typelevel/scalacheck@a2100f6 too internally - though it's overall a good thing. It's surfacing mistakes that were quietly being ignored before.

isnotinvain · 2016-12-06T20:10:30Z

Can you explain some of the Interval changes? Why was Either replaced w/ MaybeEmpty? Why does the implicit Ordering move from being a class member of Interval to a method param on each method? Having the Ordering bound at construction time instead of separately (possibly differently) at each call sight seems like a big change.

johnynek · 2016-12-07T00:37:33Z

Either now has a .contains so the old code to trigger the implicit no longer runs. So, rather than use a type we can't control, we added MaybeEmpty, so we know the implicit will get triggered.

The implicit Ordering at callsite is something we should do, IMO, but we could factor that out to a second PR.

isnotinvain · 2016-12-07T00:43:09Z

OK -- I wasn't saying it should be one way or another. I don't really understand this enough to have an opinion so that's why I was asking for some clarification for my own understanding. The Either part makes sense.

Can you clarify why an object should have its ordering provided to each of its methods instead of once to its constructor? That means an instance will behave differently depending on where it's called from which surprised me.

johnynek · 2016-12-07T02:59:52Z

@isnotinvain by capturing the typeclass, we have two issues:

serialization is painful, since you need to deal with this captured Ordering.
if you have two different Interval[T], they can have the same type T but different inner Ordering[T] instances, which makes code inconsistent. This explicitly pushes the Ordering[T] up to the users for them to keep unique.

Generally, taking typeclasses where they are used is usually considered the better design these days (Haskell always does this, but it also requires typeclass uniqueness).

isnotinvain · 2016-12-07T20:55:27Z

OK. That goes counter to some "encapsulation" type intuitions I have, like if I construct an Interval, that's probably the point in time where I know what the right ordering is, and then I hand it over to some other code, I'd sort of expect it to "keep" its ordering. The case you mentioned "if you have two different Interval[T], they can have the same type T but different inner Ordering[T] instances" is more in line with what I'd expect.

But I guess as you said, if you're going the route of Interval is just a data container, and the type class contains all the behavior, then whenever you want to "do" something with an interval you have to bring your own ordering for that. So I think I get it, it's sort of like how a List() doesn't contain an ordering, but List().sort() will ask you for one.

Thanks for the explanation.

sritchie · 2016-12-07T23:52:31Z

I like the way you put it, @isnotinvain. I think this is the way to go, and cleans up the code - as I keep pushing on documentation I'll highlight this distinction so that it's clear that we're taking a stance, and folks can respond / we can change if this introduces problems anywhere. Does that work for you?

isnotinvain · 2016-12-08T00:44:46Z

Yeah SGTM!

sritchie · 2016-12-10T04:00:31Z

Looks like the BFInstance is a straight-up scala 2.12 bug:

scala/scala#5570

scala 2.12 wasn't respecting the @transient annotation on lazy val. Fixed in 2.12.1.

sritchie · 2016-12-10T04:47:49Z

algebird-test/src/test/scala/com/twitter/algebird/BloomFilterTest.scala

-      val bytesAfterSizeCalled = new String(serialize(bf))
-      assert(bytesBeforeSizeCalled == bytesAfterSizeCalled)
+      val bytesAfterSizeCalled = Bytes(serialize(bf))
+      assert(bytesBeforeSizeCalled.size == bytesAfterSizeCalled.size)


so, this started as a legit bug with @transient lazy val. I fixed that by upgrading to scala 2.12. I can see now that this does NOT serialize the dense instance... but a single byte is different in the serialized version, so equality won't work for the test. Nothing's corrupt as far as I can tell.

I changed the test to just check for size. Is this legit?

the confusing thing is that contains should be read-only. How does calling contains change the binary representation?

It instantiates the dense representation in the lazy Val. I wonder if some but gets flipped? If so seems like a regression in 2.12, but one that doesn't affect the behavior. Maybe we should make a minimal test case to see if this is true for all classes with a transient lazyval.

this is worrisome in that the transient lazy val trick is VERY commonly used to sidestep scalding serialization issues. We should probably chase it a bit or we will see massive pains on trying to merge this at scale.

sritchie · 2016-12-10T04:48:42Z

algebird-test/src/test/scala/com/twitter/algebird/CountMinSketchTest.scala

 class CmsSmallFrequencyProperty[K: CMSHasher: Gen] extends CmsFrequencyProperty[K] {
-  def exactGenerator: Gen[Vector[K]] = Gen.containerOf[Vector, K](implicitly[Gen[K]])
+  def exactGenerator: Gen[Vector[K]] =
+    Gen.nonEmptyListOf[K](implicitly[Gen[K]]).map(_.toVector)


easy fix here for the empty generators.

codecov-io · 2016-12-10T16:48:36Z

Current coverage is 80.33% (diff: 54.54%)

Merging #600 into develop will increase coverage by 0.04%

@@            develop       #600   diff @@
==========================================
  Files           113        110     -3   
  Lines          4932       4928     -4   
  Methods        4491       4493     +2   
  Messages          0          0          
  Branches        440        435     -5   
==========================================
- Hits           3960       3959     -1   
+ Misses          972        969     -3   
  Partials          0          0

Powered by Codecov. Last update 06f83f6...f805f3b

johnynek · 2016-12-11T21:10:05Z

sorry to be a pain @sritchie, and it is totally my fault too, but can we actually break the interval changes out into a separate PR. I don't want to mess up history by atomically merging those. I think we should just do the minimal in this PR to get 2.12 working (since we have some risk of introducing bugs here and we want to have a clear version to see that happen).

sritchie · 2016-12-11T21:40:59Z

@johnynek sure, I can do that tomorrow before we pair.

sritchie · 2016-12-11T22:05:18Z

okay @johnynek, backed out those changes for Interval.

johnynek

I guess we can merge and see about dealing with the bloomfilter issue as things come up.

@isnotinvain note this transient lazy val may bite scalding pretty hard at Twitter. I imagine you should look into it.

johnynek · 2016-12-11T22:35:58Z

build.sbt

+  },
+
+  scalacOptions ++= {
+    if (scalaVersion.value startsWith "2.12")


I'm not so sure we should be turning on optimization, actually. I have heard that this is best for end projects, not libraries, but I guess we can leave it as is for now.

scala 2.12

fe618ba

sritchie added this to the 0.13.0 milestone Dec 3, 2016

sritchie mentioned this pull request Dec 3, 2016

Release for Scala 2.12? #574

Closed

sritchie-stripe added 3 commits December 3, 2016 11:24

scala 2.12 compatibility

8629d9b

add mima test to other builds (commented out for now)

8803ce9

scala 212

65e206f

sritchie-stripe added 2 commits December 3, 2016 12:57

fix optimize warning

13dd4ee

fix build for 2.11

edc1a33

add sbt doge

5954835

again

8cdf22e

sritchie force-pushed the sritchie/scala212 branch from 438ae0a to 8cdf22e Compare December 3, 2016 20:19

coverage in build

c058e86

fix interval bug

ed31163

sritchie-stripe mentioned this pull request Dec 9, 2016

SpaceSaver fromBytes & toBytes (#577) #603

Merged

fix 2.11 issue

4d5d5b0

sritchie-stripe added 2 commits December 9, 2016 21:40

new 2.12

0fc4dc2

Merge branch 'develop' into sritchie/scala212

ac12427

sritchie commented Dec 10, 2016

View reviewed changes

fix non empty list

cb4092e

sritchie force-pushed the sritchie/scala212 branch from d47ab9f to cb4092e Compare December 10, 2016 04:52

remove deprecation warnings

7bebc41

sritchie force-pushed the sritchie/scala212 branch from 6f04583 to 7bebc41 Compare December 10, 2016 05:05

prep for publish

f805f3b

back out interval changes

2aa40d1

johnynek approved these changes Dec 11, 2016

View reviewed changes

sritchie merged commit d14c81a into develop Dec 11, 2016

sritchie deleted the sritchie/scala212 branch December 11, 2016 22:39

sritchie mentioned this pull request Dec 11, 2016

Remove typeclass from interval constructor #605

Merged

Add cross-build for Scala 2.12 #600

Add cross-build for Scala 2.12 #600

Uh oh!

Conversation

sritchie commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sritchie commented Dec 3, 2016

Uh oh!

johnynek commented Dec 3, 2016

Uh oh!

ianoc commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sritchie commented Dec 3, 2016

Uh oh!

sritchie commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sritchie commented Dec 3, 2016

Uh oh!

sritchie commented Dec 3, 2016

Uh oh!

johnynek commented Dec 3, 2016

Uh oh!

johnynek commented Dec 3, 2016

Uh oh!

sritchie commented Dec 3, 2016

Uh oh!

sritchie commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isnotinvain commented Dec 6, 2016

Uh oh!

isnotinvain commented Dec 6, 2016

Uh oh!

johnynek commented Dec 7, 2016

Uh oh!

isnotinvain commented Dec 7, 2016

Uh oh!

johnynek commented Dec 7, 2016

Uh oh!

isnotinvain commented Dec 7, 2016

Uh oh!

sritchie commented Dec 7, 2016

Uh oh!

isnotinvain commented Dec 8, 2016

Uh oh!

sritchie commented Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sritchie Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

johnynek Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

sritchie Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

johnynek Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

sritchie Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 80.33% (diff: 54.54%)

Uh oh!

johnynek commented Dec 11, 2016

Uh oh!

sritchie commented Dec 11, 2016

Uh oh!

sritchie commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnynek left a comment

Choose a reason for hiding this comment

Uh oh!

sritchie commented Dec 3, 2016 •

edited

Loading

ianoc commented Dec 3, 2016 •

edited

Loading

sritchie commented Dec 3, 2016 •

edited

Loading

sritchie commented Dec 3, 2016 •

edited

Loading

sritchie commented Dec 10, 2016 •

edited

Loading

codecov-io commented Dec 10, 2016 •

edited

Loading

sritchie commented Dec 11, 2016 •

edited

Loading