Don't depend on random sampling to determine function equivalence #2577
Conversation
This is a work in progress and there is a bit more work that should probably be done before merging this. However, I have already put a fair amount of time into this and wanted to see what people thought about it before pushing through to do all of the relevant work. Cats has a lot of instances for function-like types. When we go to check the laws for these types, we are required to supply an `Eq` instance. But defining equality for functions is awkward. So far we've been approaching this by generating a bunch of values and passing them into both functions and ensuring that we get the same results from both. This can produce false positives if two functions differ but we just happen to sample values that produce the same output in both. For some purposes, it isn't a big deal if we get some occasional false positives, because over many runs of the tests with different RNG seeds, we should eventually catch any discrepancies. But here be dragons. Some tests use the results of these equality checks on the left side of an implication, so a false positive in the equality check becomes a false _negative_ (and thus a build failure) in the test result. See [here](typelevel#1666 (comment)) for further discussion. This is where my adventure with this PR begins. Cats builds have been timing out quite a bit recently, so I tried to reduce the number of random values that we sample when comparing two functions for equality. While this did speed up the build a little bit, it started leading to a much higher frequency of build failures due to false negatives in tests. So I started to rethink how we determine function equivalence. Instead of relying on nondeterministic behavior for equality, we can only provide function equality for functions whose domains are small enough to exhaustively check. If two functions produce the same output for the entirety of their domain, they are equivalent. I've introduced an `ExhaustiveCheck[A]` type class that is similar to `Gen[A]` but produces a `Stream[A]` of the entire domain of `A`. I made the name somewhat specific to tests as opposed to something like `Finite[A]`, because types like `Int` have a finite domain but would be prohibitively expensive to exhaustively check in tests and therefore shouldn't have an instance for this type class. I also added some `Eq` instances for function-like types whose domains have `ExhaustiveCheck` instances. For the sake of compatibility, I didn't remove the old `Eq` instances, but I've put them in a lower implicit priority scope, and I've changed the sites that were using them to use the new instances (well not quite all of them yet -- that's why this PR isn't quite complete yet). The main benefits of this change as I see it are: 1. Remove some nondeterministic behavior from the build. 2. Allow for shrinking of the number of values checked to improve build times without triggering build failures. 3. Allow for future deprecation of some problematic instances that are exposed through cats-laws but that users should probably not depend on. The main potential downside that I can think of is that we might be checking 15 examples where we were checking 50 before, which could be considered a reduction in test coverage. However, I think that all of the places where this sort of approach is used are parametric on the type, so I don't think that it should matter much that the domain for this type is much smaller. Let me know what you think. If people like this approach then I can switch over the remaining bits.
ceedubs
requested review from
kailuowang and
johnynek
and removed request for
johnynek
| trait ExhaustiveCheck[A] extends Serializable { self => | ||
| def allValues: Stream[A] | ||
|
|
||
| def map[B](f: A => B): ExhaustiveCheck[B] = new ExhaustiveCheck[B] { |
There was a problem hiding this comment.
This isn’t safe. I think it should be imap to preserve the exhaustivity.
There was a problem hiding this comment.
Thanks, good catch!
| * Similar to `Int`, but with a much smaller domain. The exact range of [[MiniInt]] may be tuned | ||
| * from time to time, so consumers of this type should avoid depending on its exact range. | ||
| */ | ||
| final class MiniInt private (val asInt: Int) extends AnyVal with Serializable |
There was a problem hiding this comment.
Why not just use Short or Byte or something?
There was a problem hiding this comment.
Do you mean on the inside of the MiniInt or using Byte or Short directly without a wrapper?
I chose to use a custom type so that we could tune the range as desired. A lot of times these checks come up when a law is checking the equality inside of a forAll, so we'll end up with 50 items in an outer loop (based on our Scalacheck config). If we were to use byte then we'd end up with 50 * 256 comparisons (or perhaps 50 * 256 * 256 if we are comparing two Function2 instances for equality). So I thought that something with a smaller domain might be desirable.
If you were just referring to why not use Byte instead of Int inside the MiniInt class, then I have no good reason and can change if you'd like.
In case it's of interest at one point I went down a rabbit hole of reimplementing parts of smallcheck within Cats laws so that we could have tests declare the "depth" that they wanted to use when comparing for equality. But it seemed like overkill and testing over a subset of a domain of Int didn't feel as safe to me as testing against the entire domain of a smaller type.
| * domain of values as opposed to generating a random sampling of values. | ||
| */ | ||
| trait ExhaustiveCheck[A] extends Serializable { self => | ||
| def allValues: Stream[A] |
There was a problem hiding this comment.
Hmm, my thinking is that Stream as a type doesn't enforce exhaustiveness, you probably have a practical case for favoring Stream over List/Chain somewhere right?
There was a problem hiding this comment.
The Eq instances that use ExhaustiveCheck use forall, so my thought was that you wouldn't necessarily need to generate all instances for tests that fail fast. But since this is intended for small domains, there's really no reason to think that Stream would be better than List. I'm happy to change it if you'd like.
| a <- A.allValues | ||
| b <- B.allValues | ||
| c <- C.allValues | ||
| } yield (a, b, c) |
There was a problem hiding this comment.
for MiniInt, it will be 349 values, just curious do we actually need this instance or you added it preemptively?
There was a problem hiding this comment.
It is used by some tests, but I tried to make those places use (MiniInt, Boolean, Boolean) to reduce the domain size a bit. Let me know if you found a place where I failed to do that.
| * Similar to `Int`, but with a much smaller domain. The exact range of [[MiniInt]] may be tuned | ||
| * from time to time, so consumers of this type should avoid depending on its exact range. | ||
| */ | ||
| final class MiniInt private (val asInt: Int) extends AnyVal with Serializable |
There was a problem hiding this comment.
Since this MiniInt will be used as a type parameter a lot, maybe we should give https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/data/Newtype.scala a try to see if that makes an impact in test time? Of course that could be in a separate PR.
There was a problem hiding this comment.
I would imagine that this overhead would be pretty small compared to other things that tests are doing, but it's certainly something that we could explore.
|
Paging @non doesn’t this smell like Kronecker? I like Stream since it is like and immutable iterator. You can avoid holding everything in memory by not holding the head. Not the case for List or Chain. |
|
Ah that's right, I was pretty sure that Erik had done something like this, but I didn't look in any of the right repositories! |
|
I took a look, and this does seem to be doing a (fairly small) subset of what Kronecker does. However, Kronecker has a few dependencies including Spire which would introduce a cyclical dependency were we to have cats-laws depend on Kronecker. I'm inclined to think that |
|
build fails a serialization test on 2.13
I am not sure but it could be related to scala/scala-dev#562 |
|
I don't expect to have time to work on this for the next couple of weeks. Once I have some availability should I finish out this effort? So far it sounds like Kai might be a yes vote, but I'd like to have more buy-in before continuing. |
|
Okay it looks like we have at least a couple of maintainers onboard for this sort of change. I'll work through the remaining pieces as I find the time and update this PR. |
|
FWIW I ran into #2701 while working on this, so I put this effort on hold until that was resolved. But if we can't solve that one in a binary-compatible way then I can probably work around it in the tests here.
|
Codecov Report
@@ Coverage Diff @@
## master #2577 +/- ##
==========================================
- Coverage 95.14% 94.47% -0.67%
==========================================
Files 365 367 +2
Lines 6816 6877 +61
Branches 302 303 +1
==========================================
+ Hits 6485 6497 +12
- Misses 331 380 +49
Continue to review full report at Codecov.
|
|
Hmm strange. The build is failing with a compile error that I don't get locally. I'm not sure what's going on here. Edit: never mind it looks like it must have been building off of some changes that have been included in master? I'll merge master into this branch again and resolve. |
|
Okay I think that this PR is ready for review again. Some of it ended up getting a bit involved; for example the CSV Codec tests were checking equality of CSV codecs, which isn't feasible to do in an exhaustive way. I changed them to use a much more constrained list with a max of 6 bits. The code coverage drop is because I've left all of the old equality instances around (but deprecated), but the tests aren't hitting them. I guess that I could add some tests specifically for them if people would like me to do that; but I'm not even sure how to call them without fatal deprecation warnings. |
|
@LukaJCB would you be up for taking a look at this now that it's updated? |
In order to test Invariant[Fractional], we need some way to define an Eq[Invariant[Fractional]] for some fractional type. We do this using Float as the fractional type, but this requires the use of the deprecated cats.laws.discipline.DeprecatedEqInstances#catsLawsEqForFn1. If we instead use MiniFloat, we can take advantage of ExhaustiveCheck[MiniFloat] to use cats.laws.discipline.eq#catsLawsEqForFn1Exhaustive. See typelevel#2577 for a broader explanation of this.
This is a work in progress and there is a bit more work that should
probably be done before merging this. However, I have already put a fair
amount of time into this and wanted to see what people thought about it
before pushing through to do all of the relevant work.
the problem
Cats has a lot of instances for function-like types. When we go to check
the laws for these types, we are required to supply an
Eqinstance.But defining equality for functions is awkward. So far we've been
approaching this by generating a bunch of values and passing them into
both functions and ensuring that we get the same results from both. This
can produce false positives if two functions differ but we just happen
to sample values that produce the same output in both. For some
purposes, it isn't a big deal if we get some occasional false positives,
because over many runs of the tests with different RNG seeds, we should
eventually catch any discrepancies.
But here be dragons. Some tests use the results of these equality checks
on the left side of an implication, so a false positive in the equality
check becomes a false negative (and thus a build failure) in the test result.
See here for further discussion.
This is where my adventure with this PR begins. Cats builds have been
timing out quite a bit recently, so I tried to reduce the number of
random values that we sample when comparing two functions for equality.
While this did speed up the build a little bit, it started leading to a
much higher frequency of build failures due to false negatives in tests.
the proposed solution
I started to rethink how we determine function equivalence. Instead
of relying on nondeterministic behavior for equality, we can only
provide function equality for functions whose domains are small enough
to exhaustively check. If two functions produce the same output for the
entirety of their domain, they are equivalent.
I've introduced an
ExhaustiveCheck[A]type class that is similar toGen[A]but produces aStream[A]of the entire domain ofA. I madethe name somewhat specific to tests as opposed to something like
Finite[A], because types likeInthave a finite domain but would beprohibitively expensive to exhaustively check in tests and therefore
shouldn't have an instance for this type class.
I also added some
Eqinstances for function-like types whose domainshave
ExhaustiveCheckinstances. For the sake of compatibility, Ididn't remove the old
Eqinstances, but I've put them in a lowerimplicit priority scope, and I've changed the sites that were using them
to use the new instances (well not quite all of them yet -- that's why
this PR isn't quite complete yet).
benefits
The main benefits of this change as I see it are:
times without triggering build failures.
exposed through cats-laws but that users should probably not depend on.
downsides
The main potential downside that I can think of is that we might be
checking 15 examples where we were checking 50 before, which could be
considered a reduction in test coverage. However, I think that all of
the places where this sort of approach is used are parametric on the
type, so I don't think that it should matter much that the domain for
this type is much smaller.
request for feedback
Let me know what you think. If people like this approach then I can
switch over the remaining bits.