-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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-11295 Add packages to JUnit output for Python tests #9263
Conversation
Jenkins, this is ok to test. |
Test build #44299 has finished for PR 9263 at commit
|
Test build #44300 has finished for PR 9263 at commit
|
pyspark.tests shows up https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/44300/testReport/ I will work the others. |
Test build #44302 has finished for PR 9263 at commit
|
The test errors with:
|
Test build #44310 has finished for PR 9263 at commit
|
@@ -76,7 +76,8 @@ | |||
pass | |||
|
|||
ser = PickleSerializer() | |||
sc = SparkContext('local[4]', "MLlib tests") | |||
conf = SparkConf().set("spark.driver.allowMultipleContexts", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it relevant to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mengxr This was my (failing) attempt to correct the test errors (caused by --parallelism=4
?). Maybe @JoshRosen could comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you shouldn't set this. The parallelism in dev/run-tests
will actually launch separate JVMs, so that's not the cause of this problem. In general, you should never set spark.driver.allowMultipleContexts
(it was only added as an escape-hatch backwards-compatibility option for a feature that we never properly supported).
There must be some other problem in the tests, likely due to test cleanup or SparkContext teardown not being executed properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing the tests.py-s
https://github.com/apache/spark/blob/master/python/pyspark/streaming/tests.py
initiates SparkContext differently:
@classmethod
def setUpClass(cls):
class_name = cls.__name__
conf = SparkConf().set("spark.default.parallelism", 1)
cls.sc = SparkContext(appName=class_name, conf=conf)
cls.sc.setCheckpointDir("/tmp")
@classmethod
def tearDownClass(cls):
cls.sc.stop()
# Clean up in the JVM just in case there has been some issues in Python API
try:
jSparkContextOption = SparkContext._jvm.SparkContext.get()
if jSparkContextOption.nonEmpty():
jSparkContextOption.get().stop()
except:
pass
Could this approach be retrofitted into https://github.com/apache/spark/blob/master/python/pyspark/mllib/tests.py to allow for concurrency?
Test build #44478 has finished for PR 9263 at commit
|
This last run had a different failure than the previous run with the same code ... |
@JoshRosen Would you have some pointers on how to move this forward? Thanks |
test this please |
add to whitelist |
Test build #45227 has finished for PR 9263 at commit
|
@gliptak I think this is the cause: https://github.com/apache/spark/blob/master/python/pyspark/mllib/tests.py#L79. We didn't initialize SparkContext in |
@mengxr Thank you for the pointer. This worked locally with
|
@mengxr Could you trigger a build? |
Test build #45309 has finished for PR 9263 at commit
|
Test build #45325 has finished for PR 9263 at commit
|
Failure for only one of the Pythons version?
The unit tests page shows no errors: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45325/testReport/ |
Are JUnit test results separated based on Java version running under? |
Rebased to current master |
Test build #45936 has finished for PR 9263 at commit
|
Unit test timed out?
|
retest this please |
Test build #47554 has finished for PR 9263 at commit
|
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47554/testReport/ shows 0 failures ... |
@zsxwing Are there some changes you would like to see to this pull request? |
Hey, sorry for forgetting about this. I'm going to trigger a retest now and will take a look at the test results. @gliptak, could you please update the PR description to say something other than "WIP", since the PR description will become the commit message? Feel free to copy description from JIRA if you'd like. I'd also be nice to add a sentence or two summarizing the changes that you needed to make to get this to work. |
Jenkins, retest this please. |
@JoshRosen Please let me know if you would like to see other description changes. |
Test build #49329 has finished for PR 9263 at commit
|
@@ -75,22 +75,23 @@ | |||
# No SciPy, but that's okay, we'll skip those tests | |||
pass | |||
|
|||
ser = PickleSerializer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why sc
needs to be created on a per-test-case basis, but is it possible to leave ser
as-is and keep it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ask because it looks like this change conflicted with a recently-modified test, causing the test to break:
======================================================================
ERROR [7.323s]: test_als_ratings_id_long_error (pyspark.mllib.tests.ALSTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/mllib/tests.py", line 1571, in test_als_ratings_id_long_error
self.assertRaises(Py4JJavaError, self.sc._jvm.SerDe.loads, bytearray(ser.dumps(r)))
NameError: global name 'ser' is not defined
======================================================================
ERROR [0.779s]: test_als_ratings_serialize (pyspark.mllib.tests.ALSTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/mllib/tests.py", line 1562, in test_als_ratings_serialize
jr = self.sc._jvm.SerDe.loads(bytearray(ser.dumps(r)))
NameError: global name 'ser' is not defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall seeing some concurrency issues with ser too (it has been a while). I'm pushing up a rebase/update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by concurrency errors? AFAIK we run tests serially in these files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I was running with parallel flag locally. It has been a while ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoshRosen From the log the tests are running parallel=4.
/home/jenkins/workspace/SparkPullRequestBuilder@2/python/run-tests --modules=pyspark-core,pyspark-sql,pyspark-streaming,pyspark-mllib,pyspark-ml --parallelism=4
I will roll back the ser changes in a few.
By the way, aside from the For other reviewers: this change makes the test output much nicer in Jenkins (and thus nicer on https://spark-tests.appspot.com): One interesting thing: Jenkins seems to fail when trying to show package-level test information for the PySpark tests: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/49329/testReport/pyspark.tests/. I wonder whether this could be related to the presence of multiple XML reports from concurrent PySpark test runs with different Python versions. |
Yes, the multiple reports might play into it. I have no visibility into Jenkins to assess further. |
Test build #49349 has finished for PR 9263 at commit
|
Test build #49357 has finished for PR 9263 at commit
|
|
@JoshRosen Would you like to see some other changes? |
I'd like to see what happens if you roll back that |
@gliptak @JoshRosen I think we shouldn't block this feature because of some failed MLlib unit tests. Feel free to disable the tests in this PR and create JIRAs under components "MLlib" and "PySpark" to track them. We could fix them in a follow-up PR. |
I still maintain that we should roll back the |
Test build #49531 has finished for PR 9263 at commit
|
LGTM. Merged into master. Thanks for making this work! Btw, @gliptak please do not squash your commits into one when you push new changes. It is easier to see what you changed if you keep the old commits unmodified. Thanks! |
@mengxr I will keep that in mind. Do you usually squash commits before committing to master? Thanks |
@gliptak, our merge script automatically squashes at commit time, so there's no need for you to do it yourself. |
@JoshRosen I see. Thanks |
@gliptak I reverted the change because 0ddba6d was merged before this one and it didn't use Btw, this line needs a patch: 0ddba6d#diff-ce16909e38fc8bee429dc638b2b2dde2R426. |
SPARK-11295 Add packages to JUnit output for Python tests This improves grouping/display of test case results. Author: Gábor Lipták <gliptak@gmail.com> Closes apache#9263 from gliptak/SPARK-11295.
I made a new PR with fix: #10850 |
This is #9263 from gliptak (improving grouping/display of test case results) with a small fix of bisecting k-means unit test. Author: Gábor Lipták <gliptak@gmail.com> Author: Xiangrui Meng <meng@databricks.com> Closes #10850 from mengxr/SPARK-11295.
@mengxr Thank you (I didn't get to this last night) |
SPARK-11295 Add packages to JUnit output for Python tests
This improves grouping/display of test case results.