Use the default seed from Random to reproduce historical results.

[LTLA]

It seems that the latest Annoy library (in single-threaded mode) yields different results to the previous version, due to a change in the seed used for tree construction. The previous Annoy version initialized Random with its default seed, while the current version effectively sets it to zero without manual intervention:

annoy/src/annoylib.h

Lines 1144 to 1146 in ce7d767

	// Each thread needs its own seed, otherwise each thread would be building the same tree(s)
	int seed = _is_seeded ? _seed + thread_idx : thread_idx;
	_random.set_seed(seed);

(i.e., if I don't call set_seed() somewhere along the line, and I use the single-threaded policy, then _seed ends up being zero due to _is_seeded=false and thread_idx=0.)

This change is a bit - ahem - annoying for scientific applications where we would like to be able to reproduce the result of computational workflows. I mean, I get it, sometimes algorithms have to change and that different results are unavoidable; but in this case, the change does not seem necessary. One seed is as good as any other, and the old seed does look, at least, a bit better than just a flat zero.

This PR modifies the Annoy C++ library to recover the old results by ensuring that - for single-threaded applications - we are using the Random's default seed. It modifies a few interfaces to ensure that the user can actually pass large uints to Random without truncation to int. Furthermore, we streamline the class a little by removing the need for the is_seeded indicator.

Some notes:

• Apologies for the clunky type detection, I couldn't figure out a more succint way to do that.
• The new template parameter for AnnoyIndexInterface defaults to uint64_t under the assumption that most people are using Kiss64Random and thus any C++ code referring to AnnoyIndexInterfaces (e.g., in pointers to the base class) should continue to work.
• Classes for Random now have the extra requirement that a default_seed variable be present. This may affect some users who are passing in their own PRNGs.
• Wasn't quite sure what to do with the Python wrapper around set_seed, which effectively truncates to int. RcppAnnoy has the same problem.

annoy/src/annoymodule.cc

Lines 515 to 525 in ce7d767

	py_an_set_seed(py_annoy *self, PyObject *args) {
	int q;
	if (!self->ptr)
	return NULL;
	if (!PyArg_ParseTuple(args, "i", &q))
	return NULL;
	self->ptr->set_seed(q);
	Py_RETURN_NONE;
	}

Further context on this problem can be found in jlmelville/uwot#69.

[erikbern]

Thanks! I just realized there's a risk that with multithreaded build, it still won't be perfectly deterministic, since it might also depend on the timing of threads. But you said you're running it in single-threaded mode?

[LTLA]

Yep, that's right. A few months ago, @eddelbuettel and I were testing out the multi-threaded version of the index building for RcppAnnoy but realized that it gave different results to the single-threaded version, so we decided to stick to a single thread.

FWIW I implement reproducible multi-threaded randomization by defining one seed for each task - in this case, building each tree. So the first tree gets a seed of _seed + 0, the second tree gets a seed of _seed + 1, and so on. This ensures that the task of generating the requested number trees can be arbitrarily divided among any number of threads without affecting the final result. One could imagine doing something like this for the AnnoyIndexMultiThreadedBuildPolicy.

Anyway, the CI failures seem to arise from the lack of C++11 in some of the builds. I don't know whether requiring C++11 would be an imposition in this day and age, but I suppose we could switch out the decltype stuff for a hard-coded type specification.

[eddelbuettel]

IIRC the multithreaded code added in 1.17 even requires C++14 so C++11 is not really a "constraint" there.

We should just do the right thing for this advanced use, and also leave the sequential mode in "conservative" build settings for those CentOS 7 / RHEL 7 users with a compiler from the Cambrian age.

[LTLA]

After some struggle, I eventually remembered how to code against the C++03 standard. The latest version of this PR will allow the library to work on older compilers, though in such cases, a custom Random has the additional requirement of providing a typedef for seed_type to specify the unsigned integer type.

The per-tree seeds relate to a different problem as they will not reproduce historical results, just provide consistency across different parallelization schemes; so I will leave that for a different PR, if that is of interest.

[erikbern]

thanks for the PR. is this ready to be merged?

[LTLA]

Yep, fine with me.

[eddelbuettel]

"What he said" -- I have no irons in this fire and trust Aaron on this.

[erikbern]

Is there some way to add a unit test for this btw?

[LTLA]

In theory - yes - we hard-code a search result into the test file (e.g., nearest neighbor indices for the first 20 observations in some test dataset, e.g., MNIST) and we compare all subsequent searches to that hard-coded result. Not the cleanest, but it will at least guarantee that we're getting back the same results as we did before. Probably a good idea to check that a different seed will give a different result, to make sure we don't accidentally make a self-defeating unit test that is resistant to stochasticity.

I guess I'll try to put something together tonight.

[LTLA]

Well, that was a faceful of reality. I didn't notice that the Python module is compiled with multi-threading on, so any unit tests for consistency of the single-threaded policy won't apply there. In theory, the tests would be relevant for the Python 2 builds, but even then the results don't agree with what I get on my computer; I don't know why. Some combination of compiler settings, perhaps, causing changes in precision that are particularly impactful on the float default.

So I guess the actual answer to the question is: no, the unit tests to check this behavior are too fragile. (Makes me a little concerned about whether RcppAnnoy is giving the same result across platforms, though testing suggests that clang and g++ agree.)

[erikbern]

it's ok, let's merge this for now! thanks!

[kaingwade]

FWIW I implement reproducible multi-threaded randomization by defining one seed for each task - in this case, building each tree. So the first tree gets a seed of _seed + 0, the second tree gets a seed of _seed + 1, and so on. This ensures that the task of generating the requested number trees can be arbitrarily divided among any number of threads without affecting the final result.

The multi-threaded build is still not perfectly deterministic. I'm thinking of the same strategy as yours. Is their any chance that you could create a PR @LTLA, since you have already implemented it?

[LTLA]

Hm... jogging my memory, I don't think I ever actually implemented it for Annoy. I think I was speaking about my implementation for some other projects, and was just extrapolating that approach to Annoy. It shouldn't be too hard to it but you might as well start fresh because even if I had ever written it up, I suspect that particular branch is long gone.