Github user WeichenXu123 commented on the issue:
https://github.com/apache/spark/pull/16774
OK. I will separate a PR. Thanks!
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
@WeichenXu123 , it would be great if you could separate out the bugfix. I
looked in #19208 but couldn't find what you were referring to.
---
Github user jkbradley commented on the issue:
https://github.com/apache/spark/pull/16774
@WeichenXu123 Thanks for finding that bug! Can you please separate out
your bugfix? It's good to get fixes in, rather than attaching them to PRs
which may require discussion, so that we make
Github user WeichenXu123 commented on the issue:
https://github.com/apache/spark/pull/16774
@BryanCutler @MLnick I found a bug in this PR: after save estimator (CV or
TVS) and then load again, the "Parallelism" setting will be lost. But I fix
this in #19208 by the way.
---
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/16774
LGTM. Merged this to master. Thanks @BryanCutler and everyone for reviewing!
(in this PR the doc in the trait is a little more detailed which is
slightly better IMO).
---
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
This should be ready to merge @jkbradley @MLnick
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81417/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #81417 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81417/testReport)**
for PR 16774 at commit
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
If you wouldn't mind, please merge this first (once passes Jenkins). I had
already updated this with the HasParallelism trait after waiting on OneVsRest
for a while. Thanks!
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #81417 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81417/testReport)**
for PR 16774 at commit
Github user jkbradley commented on the issue:
https://github.com/apache/spark/pull/16774
I think https://github.com/apache/spark/pull/19110 is ready to merge now.
@BryanCutler @WeichenXu123 do you have a preference for which gets merged first?
---
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
ping @MLnick , does this look ok to merge?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
@MLnick @WeichenXu123 I updated to use the trait `HasParallel` and fixed up
some of the docs, please take another look, thanks!
---
If your project is set up for it, you can reply to this
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81047/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #81047 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81047/testReport)**
for PR 16774 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81046/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #81046 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81046/testReport)**
for PR 16774 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #81047 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81047/testReport)**
for PR 16774 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #81046 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81046/testReport)**
for PR 16774 at commit
Github user WeichenXu123 commented on the issue:
https://github.com/apache/spark/pull/16774
@BryanCutler @MLnick I agree pick `HasParallel` into this PR because the
`trait` has very little code. Another feature is pending on this PR. So we hope
this get merged soon! cc @jkbradley
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/16774
@BryanCutler can we pick the `HasParallel` trait from #18281 into this PR?
Then this PR won't be blocked, and when #18281 is ready it should still be
cleanly mergeable.
---
If your project
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #3896 has
started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3896/testReport)**
for PR 16774 at commit
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
Thanks for taking a look @WeichenXu123!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this
Github user WeichenXu123 commented on the issue:
https://github.com/apache/spark/pull/16774
@BryanCutler You are right. Once `Future` complete the model can be cleaned
by GC. So the memory cost of the code has been optimized already. I didn't look
at the code carefully a few days
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
@WeichenXu123 , if #18733 is ready to be merged, then it should not be put
on hold because of this PR. We also want to make sure to work collaboratively
if there is overlap, and respect other
Github user hhbyyh commented on the issue:
https://github.com/apache/spark/pull/16774
I'm confused by your suggestions here and in #18733.
I don't think it's appropriate to just "include" a similar work originated
from another PR, and suggest another PR to suspend.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78114/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #78114 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78114/testReport)**
for PR 16774 at commit
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
retest this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #78018 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78018/testReport)**
for PR 16774 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78018/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #78018 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78018/testReport)**
for PR 16774 at commit
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
Thanks for getting back to this @MLnick. The only difference with daemon
threads is they won't prevent the JVM from shutting down - which is what we
would want in any case. The name is a bit
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/16774
@BryanCutler getting back to this after a long delay now that `2.2` is
about ready. Sorry about that!
You mentioned the approach above for creating the `ExecutorService` being
useful
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/75716/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #75716 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75716/testReport)**
for PR 16774 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #75716 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/75716/testReport)**
for PR 16774 at commit
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
Thanks for the review @MLnick! I changed `setExecutorService` to use a
trait instead of just a function, which can be implemented in Java. Works the
same, but does add the public trait if
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/73549/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #73549 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/73549/testReport)**
for PR 16774 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/73545/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16774
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #73545 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/73545/testReport)**
for PR 16774 at commit
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
@thunterdb and @MLnick I updated this to use a configurable
`ExecutorService` and `Future`s instead of Scala parallel collections. The
ExecutorService is retrieved by a function to lazily
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #73549 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/73549/testReport)**
for PR 16774 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16774
**[Test build #73545 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/73545/testReport)**
for PR 16774 at commit
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
Hi @thunterdb , thanks for the review and all of the details you provided!
I agree that a configurable execution service would be needed for running under
a shared environment instead of
Github user thunterdb commented on the issue:
https://github.com/apache/spark/pull/16774
Thanks for working on this task, this is a much requested feature. While it
will work for simple cases in the current shape, it is going to cause some
issues for any complex deployments (Apache
Github user BryanCutler commented on the issue:
https://github.com/apache/spark/pull/16774
@MLnick , I updated this PR:
* added unit tests
* added usage of parameter to examples
* updated ml-tuning.md documentation
* changed sliding window to a bounded semaphore to
56 matches
Mail list logo
