Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix doctests #165

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Fix doctests #165

merged 1 commit into from
Jul 30, 2024

Conversation

sol
Copy link
Member

@sol sol commented Jul 29, 2024

@@ -956,7 +956,7 @@ class Uniform a where
--
-- There is a default implementation via 'Generic':
--
-- >>> :set -XDeriveGeneric -XDeriveAnyClass
-- >>> :seti -XDeriveGeneric -XDeriveAnyClass
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bodigrim :set sets options for source files and ghci expressions, while :seti only affects ghci expressions.

  1. With :set those three tests were failing, I think due to a reincarnation of https://gitlab.haskell.org/ghc/ghc/-/issues/20670.
  2. For doctests you usually want :seti anyway.

@sol sol mentioned this pull request Jul 29, 2024
10 tasks
@idontgetoutmuch
Copy link
Member

Why do the generated random numbers change? I would have thought with a fixed seed we should always get the same random numbers i.e. why this: https://github.com/haskell/random/pull/165/files#diff-fe93ae678c398cfea72ff60186cc2cee27bd752aad99d8fd1ece059c912f0104L645

@sol
Copy link
Member Author

sol commented Jul 29, 2024

@idontgetoutmuch I don't think the seed was fixed for those examples before this PR.

@lehins
Copy link
Contributor

lehins commented Jul 29, 2024

Why do the generated random numbers change? I would have thought with a fixed seed we should always get the same random numbers i.e. why this: https://github.com/haskell/random/pull/165/files#diff-fe93ae678c398cfea72ff60186cc2cee27bd752aad99d8fd1ece059c912f0104L645

@idontgetoutmuch That is because that example uses global StdGen, which as comment below example points out, is usually not a very good idea:

random/src/System/Random.hs

Lines 647 to 650 in 623cf51

-- This is an outdated function and it is recommended to switch to its
-- equivalent 'System.Random.Stateful.applyAtomicGen' instead, possibly with the
-- 'System.Random.Stateful.globalStdGen' if relying on the global state is
-- acceptable.

@@ -354,7 +354,7 @@ class Random a where
-- independently:
--
-- >>> fst $ randomR (('a', 5.0), ('z', 10.0)) $ mkStdGen 2021
-- ('t',6.240232662366563)
-- ('t',6.240232662366564)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idontgetoutmuch I am more curious to see floating point number generation be affected by the change of how doctests are being executed. I suspect this is coming from some ghc optimizations, so I don't think we'll be able to do anything about it, but it could be worth investigating where exactly it is coming from.

Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing doctest for us @sol. I greatly appreciate it.
I could never figure out what was wrong with our current setup.
Could I ask you to change the CI in such a way that we run doctests for many ghc versions, instead of just one. This is quite important for making sure our examples also work the same for users of all supported ghc versions.

@Shimuuar
Copy link
Contributor

In fact we do fix seed for doctests. In module System.Random.Stateful:

-- $setup
-- >>> import Control.Monad.Primitive
-- >>> import qualified System.Random.MWC as MWC
-- >>> writeIORef theStdGen $ mkStdGen 2021

Maybe cabal doctest executes modules in different order?

@lehins
Copy link
Contributor

lehins commented Jul 30, 2024

Maybe cabal doctest executes modules in different order?

I am sure that is the case. Which is also the reason why I always suggest avoid using the global generator whenever reproducability is important.

@lehins lehins merged commit 4b45e3a into haskell:master Jul 30, 2024
49 of 50 checks passed
@sol sol deleted the fix-doctests branch July 31, 2024 04:56
@sol sol restored the fix-doctests branch July 31, 2024 04:56
@sol sol deleted the fix-doctests branch July 31, 2024 04:59
@sol sol restored the fix-doctests branch July 31, 2024 04:59
@sol sol deleted the fix-doctests branch July 31, 2024 04:59
@lehins lehins mentioned this pull request Oct 27, 2024
@juhp
Copy link

juhp commented Dec 11, 2024

Are there any release plans, btw?
Asking, since Stackage is still stuck on an older doctest version because of this.

@lehins
Copy link
Contributor

lehins commented Dec 11, 2024

Are there any release plans, btw?

@juhp Yes, I am planning on making a release over Christmas break.

In any case, I suspect stackage will be stuck on this for a while, since last time major version of random was released it took stackage almost a year to adopt it (all due tons of speculative upper bounds in downstream packages: commercialhaskell/stackage#5474)

I can try making a patch release of random-1.2.1.3 that drops usage of doctest, in order to unblock doctes. Would that be helpful?

@juhp
Copy link

juhp commented Dec 12, 2024

I can try making a patch release of random-1.2.1.3 that drops usage of doctest, in order to unblock doctes. Would that be helpful?

Yes that sounds good if you can, thank you!

@juhp
Copy link

juhp commented Dec 12, 2024

For now I try to disable the random testsuite in Stackage Nightly as a workaround hopefully

juhp added a commit to commercialhaskell/stackage that referenced this pull request Dec 12, 2024
lehins added a commit that referenced this pull request Dec 14, 2024
Starting with `random-1.3.0` doctests are executed in a CI by invoking
`doctest` executable directly, which turns out to be the correct way to
use doctest. This was pointed out in #165

In order to avoid `random-1.2.x` major version depending on doctest, this
patch removes doctest usage completely, in favor of a new setup going
forward. However, in order to prevent tooling breakaage downstream that
runs tests we do not remove the `doctest` executable section.
@lehins lehins mentioned this pull request Dec 14, 2024
@lehins
Copy link
Contributor

lehins commented Dec 14, 2024

Yes that sounds good if you can, thank you!

@juhp random-1.2.1.3 that does not depend on doctest is up on Hackage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants