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-22993][ML] Clarify HasCheckpointInterval param doc #20188

Closed
wants to merge 4 commits into from

Conversation

sethah
Copy link
Contributor

@sethah sethah commented Jan 8, 2018

What changes were proposed in this pull request?

Add a note to the HasCheckpointInterval parameter doc that clarifies that this setting is ignored when no checkpoint directory has been set on the spark context.

How was this patch tested?

No tests necessary, just a doc update.

@sethah
Copy link
Contributor Author

sethah commented Jan 8, 2018

cc @srowen @holdenk The MLlib counterparts actually make mention of this, but for some reason the note never got ported over to ML package.

The only caveat I can think of is that this doc is the same for all algos that inherit it, but a new algo could potentially not ignore it, but throw an error or manually set the checkpoint dir. For now, ALS, LDA, and GBT all use it and ignore it in the case the checkpoint dir is not set.

@SparkQA
Copy link

SparkQA commented Jan 8, 2018

Test build #85807 has finished for PR 20188 at commit 752d0ba.

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

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sethah
Copy link
Contributor Author

sethah commented Jan 9, 2018

Good call @felixcheung! Will update shortly.

@SparkQA
Copy link

SparkQA commented Jan 9, 2018

Test build #85881 has finished for PR 20188 at commit 3e40f76.

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

@felixcheung
Copy link
Member

felixcheung commented Jan 9, 2018 via email

@sethah
Copy link
Contributor Author

sethah commented Jan 9, 2018

Thanks, latest commit should fix it.

@SparkQA
Copy link

SparkQA commented Jan 10, 2018

Test build #85886 has finished for PR 20188 at commit c966c0c.

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

@felixcheung
Copy link
Member

merged to master/2.3

asfgit pushed a commit that referenced this pull request Jan 10, 2018
## What changes were proposed in this pull request?

Add a note to the `HasCheckpointInterval` parameter doc that clarifies that this setting is ignored when no checkpoint directory has been set on the spark context.

## How was this patch tested?

No tests necessary, just a doc update.

Author: sethah <[email protected]>

Closes #20188 from sethah/als_checkpoint_doc.

(cherry picked from commit 70bcc9d)
Signed-off-by: Felix Cheung <[email protected]>
@asfgit asfgit closed this in 70bcc9d Jan 10, 2018
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.

5 participants