Skip to content

Conversation

@MrBago
Copy link
Contributor

@MrBago MrBago commented Dec 15, 2017

What changes were proposed in this pull request?

pyspark.ml.tests is missing a py4j import. I've added the import and fixed the test that uses it. This test was only failing when testing without Hive.

How was this patch tested?

Existing tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

@srowen
Copy link
Member

srowen commented Dec 15, 2017

@HyukjinKwon I think you might have touched that code last

@SparkQA
Copy link

SparkQA commented Dec 15, 2017

Test build #84980 has finished for PR 19997 at commit 46410f1.

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

@HyukjinKwon
Copy link
Member

@MrBago, I think you can just skip when Hive support is disabled if this matters.That test is valid only with a Hive support.

import numpy as np
from numpy import abs, all, arange, array, array_equal, inf, ones, tile, zeros
import inspect
import py4j
Copy link
Member

Choose a reason for hiding this comment

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

BTW, mind elaborating how importing this fixes an issue? It sounds orthogonal to me.

Copy link
Contributor Author

@MrBago MrBago Dec 16, 2017

Choose a reason for hiding this comment

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

On the line below, we catch py4j.protocol.Py4JError so that we can then raise SkipTest instead, but if we don't import py4j we get a NameError instead of skipping the test. Furthermore, because we don't trigger tearDownClass() on the following line we leave behind stale state which causes other tests to fail. The except line is only ever triggered in environments that don't have Hive and should skip this test.

https://github.com/apache/spark/pull/19997/files#diff-4a75aace12688903bc8f97e7930622f4R1868

Copy link
Member

@HyukjinKwon HyukjinKwon Dec 16, 2017

Choose a reason for hiding this comment

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

Ah, it was my bad. Yup, you are right.

It was also written in the JIRA as well. Sorry, I just got up (I am in Korea :) ..) and rushed to leave some comments.

@HyukjinKwon
Copy link
Member

Merged to master.

@asfgit asfgit closed this in 0c8fca4 Dec 16, 2017
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