Skip to content

Conversation

@holdenk
Copy link
Contributor

@holdenk holdenk commented May 5, 2016

What changes were proposed in this pull request?

Mark ml.classification algorithms as experimental to match Scala algorithms, update PyDoc for for thresholds on LogisticRegression to have same level of info as Scala, and enable mathjax for PyDoc.

How was this patch tested?

Built docs locally & PySpark SQL tests

@SparkQA
Copy link

SparkQA commented May 5, 2016

Test build #57925 has finished for PR 12938 at commit c72fa46.

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

@holdenk holdenk changed the title [SPARK-15162][SPARK-15164][PySpark][DOCS][ML] update some pydocs [SPARK-15136][SPARK-15162][SPARK-15164][PySpark][DOCS][ML] update some pydocs May 6, 2016
@holdenk holdenk changed the title [SPARK-15136][SPARK-15162][SPARK-15164][PySpark][DOCS][ML] update some pydocs [SPARK-15162][SPARK-15164][PySpark][DOCS][ML] update some pydocs May 6, 2016
@SparkQA
Copy link

SparkQA commented May 9, 2016

Test build #58153 has finished for PR 12938 at commit 64942b7.

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

@SparkQA
Copy link

SparkQA commented May 10, 2016

Test build #58249 has finished for PR 12938 at commit a73913b.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

" Allows setting the solver: minibatch gradient descent (gd) or l-bfgs. " +
" l-bfgs is the default one.",
"Allows setting the solver: minibatch gradient descent (gd) or l-bfgs. " +
"(Default l-bfgs)",
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation is off here. Also prefer (Default: l-bfgs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, kept original indentation but will update.

@SparkQA
Copy link

SparkQA commented May 10, 2016

Test build #58255 has finished for PR 12938 at commit 9e38ddf.

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

@SparkQA
Copy link

SparkQA commented May 10, 2016

Test build #58264 has finished for PR 12938 at commit f4df8f0.

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

@SparkQA
Copy link

SparkQA commented May 19, 2016

Test build #58895 has finished for PR 12938 at commit 5df5a93.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented May 19, 2016

Test build #58901 has finished for PR 12938 at commit 2eec947.

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

@SparkQA
Copy link

SparkQA commented May 23, 2016

Test build #59158 has finished for PR 12938 at commit e11dbf8.

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

@holdenk
Copy link
Contributor Author

holdenk commented May 26, 2016

ping?

@MLnick
Copy link
Contributor

MLnick commented Jun 14, 2016

@holden does f7288e1 need to be reverted?

@holden
Copy link

holden commented Jun 14, 2016

I think you meant @holdenk

Otherwise the pr looks ok to me. ;-)

@SparkQA
Copy link

SparkQA commented Jun 14, 2016

Test build #60499 has finished for PR 12938 at commit 4431daa.

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

@holdenk
Copy link
Contributor Author

holdenk commented Jun 14, 2016

@MLnick I think its your call - as @rxin pointed out on another PR - during a release we normally revert rather than waiting to fix - but since I made a fix in the process of figuring out what the issue was I think it would also just be OK to merge this - but your call.

@holdenk holdenk changed the title [WIP][SPARK-15162][SPARK-15164][PySpark][DOCS][ML] update some pydocs [SPARK-15162][SPARK-15164][PySpark][DOCS][ML] update some pydocs Jun 14, 2016
@holdenk holdenk changed the title [SPARK-15162][SPARK-15164][PySpark][DOCS][ML] update some pydocs [SPARK-15954][SPARK-15162][SPARK-15164][PySpark][DOCS][ML][SQL] update some pydocs and fix test issue Jun 14, 2016
@MLnick
Copy link
Contributor

MLnick commented Jun 15, 2016

@holdenk master passes PySpark tests for me locally. Are we sure it is f7288e1? Is it intermittent? Because #13489 did pass Jenkins (including PySpark tests).

@holdenk
Copy link
Contributor Author

holdenk commented Jun 17, 2016

@MLnick - I think it just a timing condition - if the file is available in the resource path its all good but if it isn't then its sad. So f7288e1 would have passed, but this PR probably changes how long the pylint check takes (adds mathjax) which surfaces this.

@MLnick
Copy link
Contributor

MLnick commented Jun 17, 2016

@holdenk ok weird - now master is failing for me locally with the same error and I confirmed your fix does work.

Failure is consistent for me locally. But it doesn't appear this is impacting open/recent PySpark SQL PRs... e.g. #13717, #13558. This PR is the only place it's failing on Jenkins, which is really weird to me, since even an intermittent failure should show up on someone else's PR. So for now I have not reverted f7288e1 ... cc @rxin @sameeragarwal

In any case let's rather isolate the issue/fix in a separate PR for SPARK-15954.

@holdenk
Copy link
Contributor Author

holdenk commented Jun 17, 2016

Sounds good will make that PR :)

On Friday, June 17, 2016, Nick Pentreath [email protected] wrote:

@holdenk https://github.com/holdenk ok weird - now master is failing
for me locally with the same error and I confirmed your fix does work.

Failure is consistent for me locally. But it doesn't appear this is
impacting open/recent PySpark SQL PRs... e.g. #13717
#13717, #13558
#13558. This PR is the only place
it's failing on Jenkins, which is really weird to me, since even an
intermittent failure should show up on someone else's PR. So for now I have
not reverted f7288e1
f7288e1
... cc @rxin https://github.com/rxin @sameeragarwal
https://github.com/sameeragarwal

In any case let's rather isolate the issue/fix in a separate PR for
SPARK-15954.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12938 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AADp9V03SpADeLij0leDvXuHD3lIlODuks5qMpjngaJpZM4IYbBk
.

Cell : 425-233-8271
Twitter: https://twitter.com/holdenkarau

@MLnick
Copy link
Contributor

MLnick commented Jun 21, 2016

@holdenk can you back out 4431daa from this PR when you get a chance?

@holdenk
Copy link
Contributor Author

holdenk commented Jun 21, 2016

Sure thing.

On Tuesday, June 21, 2016, Nick Pentreath [email protected] wrote:

@holdenk https://github.com/holdenk can you back out 4431daa
4431daa
from this PR when you get a chance?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12938 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AADp9dSasgfL9zhu21irWOwssmXi0SNsks5qOABBgaJpZM4IYbBk
.

Cell : 425-233-8271
Twitter: https://twitter.com/holdenkarau

@holdenk
Copy link
Contributor Author

holdenk commented Jun 21, 2016

So this reverts the support both, but leaves in the revert of the breaking change. Did you also want that removed @MLnick ?

@mengxr
Copy link
Contributor

mengxr commented Jun 21, 2016

@holdenk @MLnick Could this PR be split into smaller ones? I don't see a good reason to put changes to HiveTest and pyspark.ml Experimental annotations under the same PR. Keeping PRs minimal is a good practice and it also helps locate issues and backport bug fixes.

@MLnick
Copy link
Contributor

MLnick commented Jun 21, 2016

I did mean by my earlier comment that this PR should focus on the doc
changes and the other new (linked) one is handling the HiveTest issue.
On Tue, 21 Jun 2016 at 20:42, Xiangrui Meng [email protected]
wrote:

@holdenk https://github.com/holdenk @MLnick https://github.com/MLnick
Could this PR be split into smaller ones? I don't see a good reason to put
changes to HiveTest and pyspark.ml Experimental annotations under the
same PR. Keeping PRs minimal is a good practice and it also helps locate
issues and backport bug fixes.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12938 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AA_SB5KowL5vQrTp_YBBbovlqU27UnI2ks5qODCugaJpZM4IYbBk
.

@holdenk
Copy link
Contributor Author

holdenk commented Jun 21, 2016

Good, I'll revert the revert then as well :)

…r reading resource files in HiveTests" as it was causing Jenkins failures."

This reverts commit 2be8cdf.
@holdenk holdenk changed the title [SPARK-15954][SPARK-15162][SPARK-15164][PySpark][DOCS][ML][SQL] update some pydocs and fix test issue [SPARK-15162][SPARK-15164][PySpark][DOCS][ML][SQL] update some pydocs and fix test issue Jun 21, 2016
@holdenk holdenk changed the title [SPARK-15162][SPARK-15164][PySpark][DOCS][ML][SQL] update some pydocs and fix test issue [SPARK-15162][SPARK-15164][PySpark][DOCS][ML] update some pydocs Jun 21, 2016
@holdenk
Copy link
Contributor Author

holdenk commented Jun 21, 2016

Stripped down to just the doc changes. I personally very much like having small PRs - the fix just happened to end up in here since it this doc change exposed the underlying issue (and it seemed like the doc change its self was small enough it would be ok to package the fix with it) as some people have asked that I group things together more.

@SparkQA
Copy link

SparkQA commented Jun 21, 2016

Test build #60943 has finished for PR 12938 at commit d842309.

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

@SparkQA
Copy link

SparkQA commented Jun 21, 2016

Test build #60949 has finished for PR 12938 at commit de63f9f.

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

@MLnick
Copy link
Contributor

MLnick commented Jun 22, 2016

Ok it's passing now :) LGTM

asfgit pushed a commit that referenced this pull request Jun 22, 2016
## What changes were proposed in this pull request?

Mark ml.classification algorithms as experimental to match Scala algorithms, update PyDoc for for thresholds on `LogisticRegression` to have same level of info as Scala, and enable mathjax for PyDoc.

## How was this patch tested?

Built docs locally & PySpark SQL tests

Author: Holden Karau <[email protected]>

Closes #12938 from holdenk/SPARK-15162-SPARK-15164-update-some-pydocs.

(cherry picked from commit d281b0b)
Signed-off-by: Nick Pentreath <[email protected]>
@MLnick
Copy link
Contributor

MLnick commented Jun 22, 2016

Merged to master/branch-2.0.

@asfgit asfgit closed this in d281b0b Jun 22, 2016
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.

7 participants