[SPARK-40514][CORE][SQL][YARN][PYTHON][TESTS] Make python related tests check python minimum support version #37956
Conversation
LuciferYang
changed the title
LuciferYang
changed the title
LuciferYang
changed the title
| @@ -285,16 +285,25 @@ private[spark] object TestUtils { | |||
| // minimum python supported version changes. | |||
| val minimumPythonSupportedVersion: String = "3.7.0" | |||
|
|
|||
| def assumePythonVersionAvailable: Unit = | |||
| assume(isPythonVersionAvailable, | |||
There was a problem hiding this comment.
assume() will skip the test if this check fails. Should we perhaps fail? otherwise one would find that Python 3.6 tests all pass?
There was a problem hiding this comment.
Spark runs on Java 8/11/17, Scala 2.12/2.13, Python 3.7+ and R 3.5+. Python 3.7 support is deprecated as of Spark 3.4.0.
Do we still promise that Python 3.0 ~ Python 3.6 can pass all tests? If not, whether it is better to skip the test and explicitly notify?
I have the impression that some cases cannot pass when using Python 3.6, let me check it again
There was a problem hiding this comment.
I have the impression that some cases cannot pass when using Python 3.6, let me check it again
Sorry, I didn't have time to check this today, will test later
There was a problem hiding this comment.
Seems Python 3.6 can pass the test, although I haven't installed and tested Pandas related cases, so we don't need to check the python version installed in the test environment? I'm not sure if the lower python version can pass the test
There was a problem hiding this comment.
Python 3.6 support isn't promised. If one runs with Python 3.6, what should we do - not test at all? fail? try anyway?
assume() will not even try to test, which seems wrong. require() would fail, which seems reasonable. It also seems reasonable to do nothing, and just try the test anyway. So if there is any change here, seems like it would be to fail explicitly, not skip the test. I could see just not doing anything, too
There was a problem hiding this comment.
The original logic is assume(TestUtils.testCommandAvailable("python3"))
So if I want to do something, it should be assume(TestUtils.testCommandAvailable("python3")) and require( py version >= 3.7)?
There was a problem hiding this comment.
That's what I'd say. Or do nothing. Just let tests try, regardless of python version
There was a problem hiding this comment.
better keep still than move, let's leave it as it is, I will close this one
| @@ -20,9 +20,9 @@ | |||
| FWDIR="$(cd "`dirname $0`"/..; pwd)" | |||
| cd "$FWDIR" | |||
|
|
|||
| PYTHON_VERSION_CHECK=$(python3 -c 'import sys; print(sys.version_info < (3, 6, 0))') | |||
| PYTHON_VERSION_CHECK=$(python3 -c 'import sys; print(sys.version_info < (3, 7, 0))') | |||
There was a problem hiding this comment.
Are these two scripts(python/run-tests, dev/run-tests) worth changing?
What changes were proposed in this pull request?
Spark 3.4 support Python 3.7+ , but python related UTs only check python executable exist, not check the python version.
So this pr make the relevant tests check whether the python executable version is greater than the minimum supported version.
Why are the changes needed?
Test python related UTs only when the minimum version support is met.
Does this PR introduce any user-facing change?
Yes, if the python runtime version is less than the minimum supported version, python related UTs will not execute.
How was this patch tested?
Pass GitHub Actions