[SPARK-19953][ML] Random Forest Models use parent UID when being fit

[BryanCutler]

What changes were proposed in this pull request?

The ML RandomForestClassificationModel and RandomForestRegressionModel were not using the estimator parent UID when being fit. This change fixes that so the models can be properly be identified with their parents.

How was this patch tested?Existing tests.

Added check to verify that model uid matches that of the parent, then renamed checkCopy to checkCopyAndUids and verified that it was called by one test for each ML algorithm.

[SparkQA]

Test build #74557 has finished for PR 17296 at commit a801ef9.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[BryanCutler]

ping @jkbradley @MLnick

[MLnick]

Seems fine - are there other instances of this happening?

I'm wondering why test cases did not pick this up... seems like we should have a standard test case for it?

[BryanCutler]

Thanks @MLnick! I checked and didn't see this happening anywhere else. It's not great to put into a test case because it requires training to get the model. It could be tacked onto existing test, but I don't know if it's really worth it. The python tests will eventually test this after SPARK-10931, which is where I discovered it.

[MLnick]

hmm I would prefer to test it though I do get it's pretty tricky to do generically. I don't think training a tiny model on a couple data points will add too much overhead.

[BryanCutler]

@MLnick , I found an existing MLTestingUtils.checkCopy that is used to check the copied model uids match and can easily be extended to include the check needed here. I went through and added these checks to any ML suite that wasn't already using, but that led to another issue that I felt should be covered in a separate PR at #17326. Can you take a look at that first and merge if ok, then I'll update this and push the regression tests I made? Thanks!

[BryanCutler]

cc @MLnick @jkbradley , I updated with the latest and added a check for the model uid to match parent. I don't think it's great that this check is tacked on to various other tests because it makes it easy to forget it if adding additional algorithms. Hopefully this is good enough for now to get this fix in and I can still follow up with another JIRA to refactor basic checks like this to make it more consistent.

[SparkQA]

Test build #75488 has finished for PR 17296 at commit dd1e3bd.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[MLnick]

High-level seems good now, though there are new conflicts in FPGrowthSuite that need to be resolved.

Did you create a JIRA to track the broader issue of trying to make the testing more generic?

Or at least - we could perhaps try to "enforce" the tests through a test trait (e.g. EstimatorModelTest) with a test that takes generated data, fits and performs the check. The trait could define an abstract generateData method. Then each concrete test could implement the data generator - most have some form of data generator method already anyway.

Of course we still need to ensure new tests implement the trait - but at least if all existing test are adapted in this way it provides the blueprint going forward.

The only other way I can think of would be via some reflection approach (but the correct form of dataset needs to be generated for each estimator...)

[SparkQA]

Test build #75551 has finished for PR 17296 at commit 7c7ce13.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[BryanCutler]

@MLnick this should be good to go. I made https://issues.apache.org/jira/browse/SPARK-20234 to address some better consistency in these basic checks.

[MLnick]

LGTM, merged to master. Thanks for creating the follow up JIRA.