r-lib / debugme

Easy and efficient debugging for R packages
https://r-lib.github.io/debugme/
Other
147 stars 10 forks source link

debugme sets .Random.seed on load #50

Closed jennybc closed 3 years ago

jennybc commented 3 years ago

I just transferred this from pillar, which is how it came into my life. But once I dug around, it became clear debugme was touching .Random.seed upon load. That explains why I talk about pillar a lot.

I picked this up because of some tests in reprex re: environment hygiene.

Something new in pillar touches the random seed, presumably something in .onLoad().

Once you figure out what it is, hopefully you can make it not so or put it inside withr::with_preserve_seed(). I don't see an obvious culprit, so is it debugme?

jenny@2020-mbp reprex $ R --vanilla

R version 4.0.2 (2020-06-22) -- "Taking Off Again"
(blah blah blah)

> ls(all.names = TRUE)
character(0)

> loadNamespace("pillar")
<environment: namespace:pillar>

> ls(all.names = TRUE)
[1] ".Random.seed"
jennybc commented 3 years ago

Same exercise when I downgrade shows no .Random.seed:

> packageVersion("pillar")
[1] ‘1.4.7’

> ls(all.names = TRUE)
character(0)

> loadNamespace("pillar")
<environment: namespace:pillar>

> ls(all.names = TRUE)
character(0)
jennybc commented 3 years ago

Interestingly, I'm only seeing the new test failure locally. Even though GHA is using pillar v1.5.0, the test doesn't fail there.

That makes me more convinced this is related to debugme, which is not available in my GHA session.

> ls(all.names = TRUE)
character(0)
> loadNamespace("debugme")
<environment: namespace:debugme>
> ls(all.names = TRUE)
[1] ".Random.seed"
jennybc commented 3 years ago

Yeah it's this, which eventually gets called whenever the debugme namespace is loaded:

https://github.com/r-lib/debugme/blob/e0e116912b159769934e4ce81bc918b9f01c4abb/R/colors.R#L12

gaborcsardi commented 3 years ago

I am not sure if this a problem. Packages are allowed to use the rng, no? If you need full reproducibility then you need to pin down the optional dependencies. Ie. either always run with debugme or always run without it.

jennybc commented 3 years ago

At this point, I've formed the impression that it's best practice not to do this. Thinking back to when ggplot2 used to touch the rng with its startup message thing, people got irritated (because, yeah, it did cause reproducibility problems that were hard to debug). So they stopped:

https://github.com/tidyverse/ggplot2/blob/9741da5050f81b7b5c012c56d02f45fc93d68f89/R/zzz.r#L1-L8

In my case, reprex suggests styler which imports tibble which imports pillar which loads debugme in .onLoad() which touches the rng. And suddenly reprex tests are failing because the global environment is not empty at the start of a reprex containing no code. It seems even worse to do it in .onLoad() vs. .onAttach().

jennybc commented 3 years ago

(And, of course, this is again related to the evaluation and leakage problems we've discussed elsewhere.)

jennybc commented 3 years ago

With the recent pillar release, this will affect every package, data analysis, etc. that uses tibble, I think.

krlmlr commented 3 years ago

Only slightly related: I've been considering enabling and disabling debugme by doing text substitution in the .R files to achieve true zero-cost logging. This process could also comment out the debugme::debugme() call in .onLoad().

For now we could call debugme() in pillar only if the DEBUGME environment variable is set, this would remove some of the pain (but also make it even harder to track down?).

jennybc commented 3 years ago

Let's assume I should deal with my reprex test issue, one way or another, which I can.

I still think it will be noticed, sooner rather than later, that library(tidyverse) or anything that causes tibble's namespace to be loaded tickles the RNG. I think it will be pretty similar to when ggplot2 used to do it. Hopefully we could design this away at the debugme level or, yes, make pillar's use of debugme somehow not trigger this.

gaborcsardi commented 3 years ago

We can fix this in debugme, probably that is the best, because it will come up again.

OTOH I don't think people can rely on packages not touching the random seed. If they want reproducibility, they have to use the same package versions, including optional packages. There is no other way.

Also, withr::with_preserve_seed() is not really a good solution for this, at least not at the debugme level, because then there is no randomness. E.g. consider we do this in debugme:

debugme_fun <- function() {
  withr::with_preserve_seed({
    sample(1:5)
  })
}
debugme_fun()
#> [1] 5 4 3 1 2
debugme_fun()
#> [1] 5 4 3 1 2

Which can be pretty bad if you are supposed to generate some random id or file name.

jennybc commented 3 years ago

I don't think people can rely on packages not touching the random seed

Agreed. I think it's different though, if merely loading a package's namespace can touch the seed. Because that can happen in very invisible and indirect ways. Like in my case, where pillar's decision to use debugme affects reprex. Even if you think it's fine, I think we know from the ggplot2 story, it's going to cause someone pain and, once they figure out who to "blame", they aren't happy 😅

I think withr::with_preserve_seed() (or inlining its logic) works fine for the actual .onLoad() application. I use it in dev reprex to permute 100 fixed adjective-animal slugs and there is definitely randomness:

devtools::load_all(".")
#> ℹ Loading reprex

head(adjective_animal)
#> [1] "soot-cub"    "fishy-coral" "cushy-kitty" "legal-mink"  "surly-egg"  
#> [6] "beton-pug"

devtools::load_all(".")
#> ℹ Loading reprex

head(adjective_animal)
#> [1] "surly-egg"   "beton-pug"   "fussy-agama" "jolly-bass"  "legal-mink" 
#> [6] "rude-stud"

Created on 2021-02-26 by the reprex package (v1.0.0.9002)

gaborcsardi commented 3 years ago

I think withr::with_preserve_seed() (or inlining its logic) works fine for the actual .onLoad() application. I use it in dev reprex to permute 100 fixed adjective-animal slugs and there is definitely randomness:

Only if there is no random seed. If there is one, then you just always start with that one, and there is no randomness. Which might be what you want, I don't know:

❯ sample(1)
[1] 1

❯ pkgload::load_all()
Loading reprex

❯ head(adjective_animal)
[1] "cocky-doe"   "waspy-bunny" "grave-cobra" "cilia-stag"  "cushy-kitty"
[6] "lucid-carp"

❯ pkgload::load_all()
Loading reprex

❯ head(adjective_animal)
[1] "cocky-doe"   "waspy-bunny" "grave-cobra" "cilia-stag"  "cushy-kitty"
[6] "lucid-carp"
jennybc commented 3 years ago

I think the typical user of reprex is not going to see the same reprex slug all the time, given how real-world usage actually happens. Personally, I attach it .Rprofile, so I'm OK 😅. I guess I'll find out.