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

[SPARK-14298] [ML] [MLlib] Add unit test for EM LDA disable checkpointing #12286

Closed
wants to merge 2 commits into from

Conversation

yanboliang
Copy link
Contributor

What changes were proposed in this pull request?

This is follow up for #12089, add unit test for EM LDA which test disable checkpointing when set checkpointInterval = -1.

How was this patch tested?

unit test.

cc @jkbradley

@SparkQA
Copy link

SparkQA commented Apr 10, 2016

Test build #55474 has finished for PR 12286 at commit acf7455.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 10, 2016

Test build #55475 has finished for PR 12286 at commit fa36cde.

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

@holdenk
Copy link
Contributor

holdenk commented Apr 11, 2016

We could maybe make this a more complete test and actually check that their are no checkpoint files generated? Although that might be over kill.

@yanboliang
Copy link
Contributor Author

@holdenk Thanks for your comments! Actually model.getCheckpointFiles.isEmpty checked that their are no checkpoint files generated.

@holdenk
Copy link
Contributor

holdenk commented Apr 11, 2016

@yanboliang that makes sense. I was thinking maybe checking the actual checkpoint directory to catch anything else (or even just clearing the checkpoint directory so checkpointing fails), but if getCheckpointFiles was correct even before the fix than that is probably a fine way to test it to.

@jkbradley
Copy link
Member

@holdenk I agree with you, but I'm worried it could fail if checkpoint deletion/cleanup does not happen quickly enough across tests in this suite. I'd prefer to leave it as is.

@jkbradley
Copy link
Member

@yanboliang Thanks! LGTM
Merging with master

@asfgit asfgit closed this in 3f0f408 Apr 11, 2016
@yanboliang yanboliang deleted the spark-14298-followup branch April 12, 2016 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants