Changed all MLContext creation to include a fixed seed

[harishsk]

Many tests are creating MLContext without a seed. Since we are seeing a number of random failures, this PR attempts to standardize all MLContext creation to include a fixed seed.

Note to reviewers: If you know if cases where a fixed seed is not desired, please flag those cases.

[justinormont]

LGTM.

We can likely tighten up the expected ranges on the unit test since the outputs will (hopefully) be more constrained now. It feels odd that I could change these seed values and no unit test would fail. We can tighten the range but not shrink to zero as deterministic results aren't expected since multi-threading causes a race condition for random number generation. I think/hope this will cause the TrainTest/CV splits to be deterministic though.

You could go a step further and feed a fixed seed to each component which takes a seed, as if I recall, they won't listen to the MLContext seed (alternatively correct this).

Certain components can be made (more/fully?) deterministic, like SDCA & LogisticRegression, by setting the number of threads to one.

Btw, 1 is a fine choice; though I prefer 42 (there's no consistency in default seed values for the repo).

[justinormont]

As discussion topic, does any know of a reason to not have a fixed seed for unit tests?

[najeeb-kazmi]

                new SdcaMaximumEntropyMulticlassTrainer.Options { MaximumNumberOfIterations = 100, Shuffle = true, NumberOfThreads = 1, }));

Shuffle = false for SDCA will give more/full determinism as Justin suggests

---

Refers to: test/Microsoft.ML.Tests/Scenarios/Api/Estimators/PredictAndMetadata.cs:34 in 4492586. [](commit_id = 4492586, deletion_comment = False)

[najeeb-kazmi]

                new SdcaMaximumEntropyMulticlassTrainer.Options { MaximumNumberOfIterations = 100, Shuffle = true, NumberOfThreads = 1 }))

Same here, and everywhere else SDCA shows up in tests.

---

Refers to: test/Microsoft.ML.Tests/Scenarios/Api/Estimators/Extensibility.cs:45 in 4492586. [](commit_id = 4492586, deletion_comment = False)

[najeeb-kazmi]

We also have this, which is used to run a lot of tests in TestPredictors.cs

machinelearning/test/Microsoft.ML.TestFramework/BaseTestPredictorsMaml.cs

Lines 341 to 344 in 3b1785c

	protected void RunAllTests(
	IList<PredictorAndArgs> predictors, IList<TestDataset> datasets,
	string[] extraSettings = null, string extraTag = "", bool summary = false,
	int digitsOfPrecision = DigitsOfPrecision, NumberParseOption parseOption = NumberParseOption.Default)

extraSettings is used to add things to the maml command line that is executed. Passing "seed=1" here would also set the seed for these.

In fact, there are several other tests that call directly one of RunOneAllTests, Run_TrainTest, or Run_CV, all of which can be passed extraSettings.

[harishsk]

Same here, and everywhere else SDCA shows up in tests.

What about cases where Shuffle has been set to true explicitly? E.g.

https://github.com/harishsk/machinelearning/blob/44925863ed727b66f344ac7c4cd90bd07528d0ee/test/Microsoft.ML.Tests/Scenarios/Api/Estimators/DecomposableTrainAndPredict.cs#L36

https://github.com/harishsk/machinelearning/blob/44925863ed727b66f344ac7c4cd90bd07528d0ee/test/Microsoft.ML.Tests/Scenarios/Api/Estimators/Extensibility.cs#L45

https://github.com/harishsk/machinelearning/blob/44925863ed727b66f344ac7c4cd90bd07528d0ee/test/Microsoft.ML.Tests/Scenarios/Api/Estimators/PredictAndMetadata.cs#L34

https://github.com/harishsk/machinelearning/blob/44925863ed727b66f344ac7c4cd90bd07528d0ee/test/Microsoft.ML.Tests/Scenarios/Api/Estimators/PredictAndMetadata.cs#L83

[najeeb-kazmi]

Same here, and everywhere else SDCA shows up in tests.

What about cases where Shuffle has been set to true explicitly? E.g.

https://github.com/harishsk/machinelearning/blob/44925863ed727b66f344ac7c4cd90bd07528d0ee/test/Microsoft.ML.Tests/Scenarios/Api/Estimators/DecomposableTrainAndPredict.cs#L36

https://github.com/harishsk/machinelearning/blob/44925863ed727b66f344ac7c4cd90bd07528d0ee/test/Microsoft.ML.Tests/Scenarios/Api/Estimators/Extensibility.cs#L45

https://github.com/harishsk/machinelearning/blob/44925863ed727b66f344ac7c4cd90bd07528d0ee/test/Microsoft.ML.Tests/Scenarios/Api/Estimators/PredictAndMetadata.cs#L34

https://github.com/harishsk/machinelearning/blob/44925863ed727b66f344ac7c4cd90bd07528d0ee/test/Microsoft.ML.Tests/Scenarios/Api/Estimators/PredictAndMetadata.cs#L83

The default is true, so I don't why it has been set explicitly. I don't see anything in the tests that would require shuffling either.

[harishsk]

Would you recommend changing those to false as well in that case?

[justinormont]

Would you recommend changing those to false as well in that case?

I'm unsure if disabling shuffling with cause the unit tests to be more stable. The shuffling and TrainTest/CV splits (hopefully) listen to seed, and would then be deterministic. Can verify by inspecting the one of the TrainTest splits, or repeatedly dumping the iDataView to a file (expecting with a fixed seed they stable perfectly the same, and changing the seed gets you a different set and ordering of rows).

[harishsk]

How about we do this in two passes then? Can we merge these changes in first and then do the shuffle=false in a second pass?

[frank-dong-ms-zz]

Do you plan to do some test whether this will improve stability?

[frank-dong-ms-zz]

[najeeb-kazmi]

How about we do this in two passes then? Can we merge these changes in first and then do the shuffle=false in a second pass?

Sounds good to me. Should we also add "seed=1" to the predictor tests in another PR or do you want to take care of this here?

[harishsk]

Do you plan to do some test whether this will improve stability?

By all consensus, seeded unit tests are the right thing to do. The tests passed here on first attempt. So I don't think this has made it any worse. I plan to run full tests against these also.