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

Tracker: planned changes for 0.5 #232

Closed
25 of 33 tasks
dhardy opened this issue Jan 12, 2018 · 50 comments
Closed
25 of 33 tasks

Tracker: planned changes for 0.5 #232

dhardy opened this issue Jan 12, 2018 · 50 comments
Assignees
Labels
P-high Priority: high X-tracker Type: tracker

Comments

@dhardy
Copy link
Member

dhardy commented Jan 12, 2018

To follow up on the release strategy in #205, here is a rough plan for Rand 0.5:

Related issues which need solving:

There are some extra changes which should be relatively easy to add after the above:

Desired additions:

Finally, a list of other planned changes, which probably won't make 0.5:

@dhardy dhardy self-assigned this Jan 12, 2018
@pitdicker
Copy link
Contributor

If we remove reseed from SeedableRng this is going to break RNG implementations. Together with the introduction of rand-core, I think it makes sense to drop the Sized requirement.

@dhardy
Copy link
Member Author

dhardy commented Jan 12, 2018

But the Sized thing only concerns Rand, which really doesn't have much to do with the Rng implementations.

@dhardy
Copy link
Member Author

dhardy commented Feb 24, 2018

Most of the above is implemented now.

There are still some changes to Range and possibly also float sampling which I believe @pitdicker is working on; it would be nice to get those in the 0.5 release, and then there is the rand-core sub-crate (#264), which could be done now or could wait until the next release (probably better now so that the impls module can be exported only from rand-core, as in my master branch).

About the release process: I think the best method might be to publish a 0.5.0-pre.0 pre-release, then wait a minimum of two weeks total for comment and testing (and at least 1 week since the last pre-release) during which no major new features will be merged.

For rand-core I guess the version number should start at 0.1.0-pre.0 and be released on the same time-schedule; after this release I don't think there will be any need to synchronise versions/releases between the two crates (and hopefully rand-core can hit 1.0 sooner).

Also during the release process we need to work out what kind of compatibility release to make for 0.4.

@dhardy
Copy link
Member Author

dhardy commented Feb 24, 2018

I just added two extra desired features: CryptoRng and switching thread_rng to HC-128.

Now would be a good time to add any extra requests for this release (but be warned that larger features may get delayed to 0.6 if not ready to merge soon).

@pitdicker
Copy link
Contributor

There are still some changes to Range and possibly also float sampling which I believe @pitdicker is working on;

Yes, copying the Range code with the RangeImp method from your branch was fairly easy (thanks to the huge merge you did 👍). I have used fixed-precision float methods as discussed in dhardy#85 (comment) for Uniform and Range. The maximum precision methods will need a little more thinking, so I'll leave them for another PR. I think it is best to merge on top of #268, because MockAddRng makes fixing the test from WeightedChoice easier (uses Range internally).

For switching thread_rng to HC-128 I have things ready, waited on #252 before making the PR.

Hopefully will finish things tomorrow.

@pitdicker
Copy link
Contributor

I think we are making pretty good progress towards 0.5 😄.

From #286 (comment)

Speaking of alternatives, there's still all your FP code in my experimental branch... have to figure out whether we'll use that at some point.

I would still like to see it become available. I think it is settled that it should change from pure conversion functions to distributions, that can use arbitrary precision (not a fixed 32 or 64 bits). So I will basically have to rewrite things. We have to come up with names, and figure out how to expose a second kind of uniform floating point ranges.

That is why I left it out of #274. Shall we leave it until after rand 0.5?

Do you still want to do the rand-core split?

One thing I really want to see is

Change the RNG used by weak_rng: dhardy#60, dhardy#52

It should not be all that much work to polish the PCG and XoroshiroMT implementations in my small_rngs crate, and also not to move all the PRNGs out to a separate crate.

@dhardy, did you somehow get a notification from this?

@dhardy
Copy link
Member Author

dhardy commented Mar 7, 2018

That is why I left it out of #274. Shall we leave it until after rand 0.5?

Perhaps; I don't think we should block 0.5 waiting on it at least.

Do you still want to do the rand-core split?

I asked, and the answer seems to be yes. This is easy to implement but I thought better to wait until most other stuff is done though; I may get to this soon now. (There's additional motivation: we can make the impls module available only through rand-core.)

I'm not sure about changing weak_rng though it definitely makes sense. Did we decide which RNG to use yet? Ultimately it would be nice for this to be in a separate crate though it could be in rand for the next release; on the other hand it should be easy to add another crate once rand-core is ready anyway.

No, I missed the notification on your history article; I'll have a read.

@pitdicker
Copy link
Contributor

Did we decide which RNG to use yet?

The choice was between PCG and my Xoroshiro variant. But to be honest I found a problem with the last one, trying to prove it was correct (a while ago actually). A widening multiply does not guarantee uniformity or equidistribution. It should be a 'real', 128 bit multiply. This can be done by using a widening multiply plus a normal multiply, causing a little slowdown (but still faster than PCG). Just to err on the safe side I would make PCG the default, and also because it is better known.

@dhardy
Copy link
Member Author

dhardy commented Mar 7, 2018

That sounds good enough to me; I'd rather err on the side of strong than fast-and-weak personally. But you should probably make a post in dhardy#60 to clarify to the people who commented there what the plan is. Also I think perhaps we should remove weak_rng and instead add a SmallRng wrapper and make users do SmallRng::new(). (We could add FastRng too and make the latter use Xoroshiro*, potentially.)

@vks
Copy link
Collaborator

vks commented Mar 7, 2018

I think removing weak_rng is a good idea. I'm not sure about the SmallRng name. Why not just use the name of the RNG? Reproducibility is important, I think changing the implementation of SmallRng should be considered a breaking change. I would rather use the name of the algorithm and recommend RNGs for different purposes in the documentation instead of abstracting them away.

In my opinion, the following kinds of generators should be considered:

  1. A cryptographic, default one. (E.g. ISAAC or HC128)
  2. A fast one without linear dependencies. (E.g. PCG or truncated xoroshiro.)
  3. A faster one with some linear dependencies. This is what practically almost everyone is currently using if cryptographic properties are not needed. (E.g. xorshift, xoroshiro)
  4. A small generator for easier seeding of larger generators from i.e. a u64. (E.g. splitmix64)
  5. A generator with a large (> 2^128) period for massively parallel computations (xorshift1024*).

The tradeoffs between 1., 2. and 3. should be clearly documented.

(Personally, I think 2. and 5. are extremely niche. 4. could be hidden behind some abstraction or replaced with 1. I don't see a convincing reason to use xoroshiro128* over xoroshiro128+.)

@dhardy
Copy link
Member Author

dhardy commented Mar 8, 2018

@vks please see dhardy#58. The intention is to provide both names (e.g. StdRng and Hc128Rng), but put the former in rand and the latter in other crates, and to clearly mark the ones in rand as non-reproducible.

"Practically almost everyone" doesn't fly as an argument; there are still many people using MT19937, and while it might be sufficient for their purposes I won't recommend it just because it's popular.

@vks
Copy link
Collaborator

vks commented Mar 8, 2018

Thanks for the link!

"Practically almost everyone" doesn't fly as an argument; there are still many people using MT19937, and while it might be sufficient for their purposes I won't recommend it just because it's popular.

In my opinion the argument has some merit, because given the widespread use of generators with linear dependencies, I would expect to hear of practical problems if there were any. Following the same line of thought, I think the statistical quality of MT is ok, the performance seems like a bigger issue in practice.

@dhardy
Copy link
Member Author

dhardy commented Mar 8, 2018

@vks I opened #289 regarding your point about dropping weak_rng. Also note:

  • the plan is to provide several good generators, by name, in sub-crates; this is however beyond the scope of the rand 0.5 release
  • we want to have a variety of reproducible generators available somehow, with minimal breakage
  • introducing one or two new generators within rand temporarily may be possible

@clarfonthey
Copy link
Contributor

I'm kind of leaning toward not having any gen methods on Rng at all and instead making these part of Distribution itself. Also, giving preferential treatment to Uniform may lead to a lot of people using it when they shouldn't, e.g. via habits from other languages, like rng.gen() % n instead of using a proper range.

@dhardy
Copy link
Member Author

dhardy commented Mar 8, 2018

Interesting idea. I wouldn't miss gen too much myself though I don't know if everyone would be happy; and we'd at least want gen_bool (or chance(p) aka Bernoulli) and gen_01 instead. I think we should leave this idea until post-0.5 however.

@clarfonthey
Copy link
Contributor

Oh, having convenience methods like bools, gen_01, and gen_range are fine, but I think gen itself should be avoided as Uniform might not be the best choice in a lot of cases.

@dhardy dhardy mentioned this issue Mar 10, 2018
5 tasks
@dhardy dhardy added P-high Priority: high X-tracker Type: tracker labels Mar 14, 2018
@dhardy
Copy link
Member Author

dhardy commented Mar 17, 2018

So, to get this back on track, what do we need to do for the 0.5 release?

  • It may be worth adding a HalfOpen01 distribution with the behaviour of the old gen() on FP types [easy]
  • We want to get the Distribution trait right: I'm trying out fn sample<R: Rng + ?Sized>(self, rng: &mut R) -> T; (copy self) which mostly works since &Distribution already implements Distribution, but there are problems with WeightedChoice; the current prototype (sample(&self, rng: &mut R)) is probably fine though
  • On the same topic, Distribution sample prototype and sized-ness of Rng #287 needs finalising

Is there anything else? It would be really nice to be able to publish 0.5.pre-0 soon!

@pitdicker
Copy link
Contributor

  • It may be worth adding a HalfOpen01 distribution with the behaviour of the old gen() on FP types

I think we should not. We have made a good simplification. Also the distinction is not really meaningful, because the change to generate exactly 0.0 is so tiny.

One other thing that I feel is important, is #293. Now that we break user code with the changes to the Rng trait, it is better to do it only once. There is only one breaking change left, deprecate gen_weighted_bool. I'll make a PR.

Personally I am in less of a hurry and would like to see some more changes. But yes, we have something nice now. Time to finish things and get a release.

@pitdicker
Copy link
Contributor

What did you think about renaming Rng to Random, and RngCore to Rng? #293 (comment)

@dhardy
Copy link
Member Author

dhardy commented Mar 17, 2018

I'm not so fussed about removing gen_weighted_bool before the next release because (a) it is independent of the other RNG changes we've made and (b) we will be leaving shuffle until later anyway. I don't see a need to rush it.

Getting the next release out — well it could wait, but

  • we've made a lot of improvements to performance and API that we're reasonably happy about now, and it's a shame to keep people waiting to use that
  • it would be really good to get feedback from users on the existing changes

Possibly a good compromise would be to make pre-releases but not make a final release yet; that way we get more testers but only those willing to put up with some more potential breaking changes.

@pitdicker
Copy link
Contributor

Shall I try to figure out the changelog?

And what do you think about releasing rand-core this weekend, and Rand 0.5 the week after that? Then RNG implementations have a little time to update.

If we want some place to host announcements/posts what do you think of https://pages.github.com/?

@dhardy
Copy link
Member Author

dhardy commented Mar 22, 2018

We already have GH pages for this repo: http://rust-lang-nursery.github.io/rand/rand/index.html
Actually, I'd forgotten that we have continually-updated published doc..

Yes, I guess we're about ready. #319 affects both rand-core and rand so needs to be merged first. #320 is purely an addition to rand and #297 purely an optimisation so both could be merged after the first pre-release, but can also be merged sooner (#320 just needs review, #297 I don't know?).

As I said before, pre-releases first then final release after early-adopters have had a chance to try this out.

@pitdicker
Copy link
Contributor

sample was deprecated in 0.4 and replaced by seq::sample_iter. Do we want to remove it already with 0.5? Asking just in case, but personally I think we should keep it for now because 0.4 did not live all that long.

@dhardy
Copy link
Member Author

dhardy commented Mar 23, 2018

I didn't see much point removing it yet either. Though for 0.6 we should definitely consider some cleanup.

@dhardy
Copy link
Member Author

dhardy commented Mar 23, 2018

I think we're ready for rand-core 0.1-pre.0 ?

rand itself is still waiting on #320 (maybe with tweaks) and #322 I guess

@dhardy
Copy link
Member Author

dhardy commented Mar 23, 2018

11:00 dhardy@localhost:~/rust/rand/rand-core$ cargo publish
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Packaging rand-core v0.1.0-pre.0 (file:///home/dhardy/rust/rand/rand-core)   
   Verifying rand-core v0.1.0-pre.0 (file:///home/dhardy/rust/rand/rand-core)
   Compiling rand-core v0.1.0-pre.0 (file:///home/dhardy/rust/rand/target/package/rand-core-0.1.0-pre.0)
    Finished dev [unoptimized + debuginfo] target(s) in 0.30 secs
   Uploading rand-core v0.1.0-pre.0 (file:///home/dhardy/rust/rand/rand-core)
error: api errors: crate was previously named `rand_core`

I regret previously releasing rand_core, and we since decided to use the name rand-core instead. Yanking didn't help. @alexcrichton any chance you can sort this? I very much doubt this was ever used.

@pitdicker
Copy link
Contributor

No reverse dependencies. As far as I know it was only used by @dhardy's experimental branch, and one or two unpublished test crates from me.

@vks
Copy link
Collaborator

vks commented Mar 23, 2018

I would probably just use rand_core.

@alexcrichton
Copy link
Contributor

@dhardy I think the rand_core name is stuck now

@dhardy
Copy link
Member Author

dhardy commented Mar 23, 2018

https://crates.io/crates/rand_core/0.1.0-pre.0

The "rand" link is broken; I guess it'll have to be an absolute link to the Crates page.

@dhardy
Copy link
Member Author

dhardy commented Mar 27, 2018

I think there's only one issue that still needs addressing before 0.5: #287. As soon as that's done I think I can make a pre-release?

@pitdicker
Copy link
Contributor

I was just thinking I have nothing left and would like to see a release 👍.

To be honest I don't see much point in pre-releases. Maybe it is good practise, but it only works when people actually use the pre-release. So I would like to see a real release, and if necessary a small point release in a few weeks.

@dhardy
Copy link
Member Author

dhardy commented Mar 27, 2018

Currently testing after updating version number: https://travis-ci.org/rust-lang-nursery/rand/builds/358840821

I'll package this as -pre.0 soon; we can still make minor modifications before the 0.5 release (#287; perhaps some Clippy suggestions, maybe doc improvements, ..)

@dhardy
Copy link
Member Author

dhardy commented Mar 27, 2018

@pitdicker
Copy link
Contributor

I think the cloudabi and fuchsia-zircon dependencies can also be shown as optional.

I had no luck installing clippy for the past few days, but wanted to run it...

@abreis
Copy link
Contributor

abreis commented Mar 27, 2018

Here's my clippy output.

@pitdicker
Copy link
Contributor

I have to admit doing a pre-release was useful! Especially thanks to the announcement on Reddit.

It seems to me we are mostly there taking care of the feedback after the pre-release. At least the part I care about 😄.

For #359 I have a branch ready. Do we want to try something with #348 now? And maybe hide the ChaChaRng::set_rounds method #349?

@dhardy
Copy link
Member Author

dhardy commented Apr 2, 2018

I have some extra docs in progress. Sorry, I've not been able to devote much time to Rand the last week (and I won't be able to next week either; might get a little time this week though).

@pitdicker
Copy link
Contributor

We are just volunteers, right?

@pitdicker
Copy link
Contributor

Found something nice in the cargo.toml that gets shipped on crates.io:

# When uploading crates to the registry Cargo will automatically
# "normalize" Cargo.toml files for maximal compatibility
# with all versions of Cargo and also rewrite `path` dependencies
# to registry (e.g. crates.io) dependencies

So we don't need to change the rand_core dependency line in cargo.toml to point to the crates.io version, we can just depend on the one in the workspace.

@rust-random rust-random deleted a comment from pitdicker Apr 5, 2018
@dhardy
Copy link
Member Author

dhardy commented Apr 5, 2018

We at least need a version number for the dependency; it complained about that last time.

@dhardy
Copy link
Member Author

dhardy commented Apr 17, 2018

rand_core 0.1.0 has been released.

@dhardy
Copy link
Member Author

dhardy commented Apr 27, 2018

I'd like to get the following done, then I think we're good for a release:

Not sure about these:

We could also look at these, but there's no reason they can't wait IMO:

@dhardy
Copy link
Member Author

dhardy commented May 18, 2018

I posted new pre-releases 3 days ago:

So far there has been little feedback regarding stuff that needs changing, so it looks likely that these will be the final pre-releases. In this case the releases should be made early next week.

@pitdicker
Copy link
Contributor

Can you make a PR with the final changes for the release, instead of a direct push (then I can be a second pair of eyes...)?

@dhardy
Copy link
Member Author

dhardy commented May 21, 2018

Looks to me like all the main things are in place for the 0.2 / 0.5 releases. So do you want to make the PR and I'll release?

@dhardy
Copy link
Member Author

dhardy commented May 21, 2018

@dhardy dhardy closed this as completed May 21, 2018
@pitdicker
Copy link
Contributor

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

@dhardy
Copy link
Member Author

dhardy commented May 22, 2018

Yes, I know! 🎉 🍰 ⛵

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high Priority: high X-tracker Type: tracker
Projects
None yet
Development

No branches or pull requests

6 participants