search

shikokuchuo / mirai

mirai - Minimalist Async Evaluation Framework for R
https://shikokuchuo.net/mirai/
GNU General Public License v3.0
185 stars 9 forks source link

Error in `nanonext::random()/2`: non-numeric argument to binary operator #75

Closed wlandau closed 1 year ago

wlandau commented 1 year ago

At some point, the return value of nanonext::random() switched from an integer to a character string. I am noticing GitHub Actions errors with targets using the r-universe builds on Mac OS GHA workflows: https://github.com/ropensci/targets/actions/runs/6124363496/job/16624225749. Which package versions should I use together?

shikokuchuo commented 1 year ago

Can you point me to where nanonext::random()/2 is in your source code? Strangely I didn't find it in either targets or crew repos.

I made the change very recently to dev nanonext as the only instances I can see it being used, it is hashed - this would allow hashing to be skipped altogether.

wlandau commented 1 year ago

The odd thing is that nanonext::random()/2 is not in the source code of either crew or targets. The only place random() shows up is in crew to generate random names: https://github.com/wlandau/crew/blob/5d915473529292f21b3e7b2d9af42a27856c1bea/R/crew_random_name.R#L13. And I suppose when the next nano next is on CRAN, I won't have to call nanonext::sha1() manually.

shikokuchuo commented 1 year ago

Yes exactly - you can eliminate the sha1() call after next nanonext. I see in crew you append random() with the current time. Actually I would just take more draws such as random(3L) or random(4L) - as you are launching the cluster at the same time so that part may actually stay the same.

So is that MacOS test picking up a very old version of targets somehow??! I can close this issue?

wlandau commented 1 year ago

So is that MacOS test picking up a very old version of targets somehow??! I can close this issue?

I had originally suspected there was an old versions of a package somewhere, but this is a workflow for the latest commit of targets. And from the session info, it appears to be using development mirai and nanonext. I have not been able to find where nanonext::random()/2 is called.

shikokuchuo commented 1 year ago

:) it does seem familiar though - I have definitely seen it before. And it's definitely in your code as I don't namespace prefix mine.

wlandau commented 1 year ago

Seems to be how the current CRAN version of crew sets seeds. For unrelated reasons, development crew no longer does this. So I think that's the reason. Should take care of itself automatically with our current release plan for mirai then crew then nanonext.

shikokuchuo commented 1 year ago

Carrying on this topic, I have re-implemented using the cryptographic random data generator in Mbed TLS. This is inspired by the function openssl::rand_bytes(), which uses openSSL as a random source, except that my version outputs the hex representation of the raw bytes rather than a raw vector so as to be more readily usable.

I have consequently amended the argument supplied to random() in mirai in preparation for the next version of nanonext, where 'n' will refer to the number of bytes generated rather than integers. I am now using random(n = 12L). In fact, even for n = 8L, there are no collisions in 10 million draws testing empirically. I am keeping this wrapped in sha1() for now so as to be compatible with current CRAN nanonext.

I suggest that you also amend the default for 'n' here in crew to prevent possible collisions once the next version of nanonext is released: https://github.com/wlandau/crew/blob/5d915473529292f21b3e7b2d9af42a27856c1bea/R/crew_random_name.R#L12