From 88c319a1c8a79c3c85807e9194a8be6c106df881 Mon Sep 17 00:00:00 2001 From: Jason Lowe Date: Fri, 12 Mar 2021 16:20:05 -0600 Subject: [PATCH 1/5] Relax cudf version check for patch-level versions Signed-off-by: Jason Lowe --- .../com/nvidia/spark/rapids/Plugin.scala | 31 ++++++++++++++- .../rapids/RapidsExecutorPluginSuite.scala | 39 +++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala index 6b6e5c6a750..81d698d77a4 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala @@ -222,7 +222,7 @@ class RapidsExecutorPlugin extends ExecutorPlugin with Logging { } val expectedCudfVersion = pluginCudfVersion.toString // compare cudf version in the classpath with the cudf version expected by plugin - if (!cudfVersion.equals(expectedCudfVersion)) { + if (!RapidsExecutorPlugin.cudfVersionSatisfied(expectedCudfVersion, cudfVersion)) { throw CudfVersionMismatchException(s"Cudf version in the classpath is different. " + s"Found $cudfVersion, RAPIDS Accelerator expects $expectedCudfVersion") } @@ -242,6 +242,35 @@ class RapidsExecutorPlugin extends ExecutorPlugin with Logging { } } +object RapidsExecutorPlugin { + /** + * Return true if the expected cudf version is satisfied by the actual version found. + * The version is satisfied if the major and minor versions match exactly. If there is a requested + * patch version then the actual patch version must be greater than or equal. + * For example, version 7.1 is not satisfied by version 7.2, but version 7.1 is satisfied by + * version 7.1.1. + */ + def cudfVersionSatisfied(expected: String, actual: String): Boolean = { + val expSplits = expected.split('.') + val actSplits = actual.split('.') + val zipSplits = expSplits.zip(actSplits) + // major and minor versions must match exactly + if (zipSplits.take(2).exists{case (e, a) => e != a}) { + return false + } + + // patch and sub-patch versions can match as long as actual is more recent + zipSplits.drop(2).forall { case (expStr, actStr) => + val expVal = Integer.parseInt(expStr) + try { + expVal <= Integer.parseInt(actStr) + } catch { + case _: IllegalArgumentException => false + } + } + } +} + object ExecutionPlanCaptureCallback { private[this] val shouldCapture: AtomicBoolean = new AtomicBoolean(false) private[this] val execPlan: AtomicReference[SparkPlan] = new AtomicReference[SparkPlan]() diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala new file mode 100644 index 00000000000..e2731dba897 --- /dev/null +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2021, NVIDIA CORPORATION. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.nvidia.spark.rapids + +import org.scalatest.FunSuite + +class RapidsExecutorPluginSuite extends FunSuite { + test("cudf version check") { + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7", "7")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7", "8")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7", "7.2")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7", "8.7")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7", "7.2.1")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0", "7.0")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0", "7.0.1")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0", "7.0.1.3")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.1")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.1.3")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.2")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.2.3")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.0")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.1")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.1.1")) + } +} From 76f1fdea1c67a72db2d154a9a74b62b4f6514a56 Mon Sep 17 00:00:00 2001 From: Jason Lowe Date: Fri, 12 Mar 2021 16:51:54 -0600 Subject: [PATCH 2/5] whitespace --- sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala index 81d698d77a4..e02b1c37db2 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala @@ -255,7 +255,7 @@ object RapidsExecutorPlugin { val actSplits = actual.split('.') val zipSplits = expSplits.zip(actSplits) // major and minor versions must match exactly - if (zipSplits.take(2).exists{case (e, a) => e != a}) { + if (zipSplits.take(2).exists{ case (e, a) => e != a }) { return false } From 215443ca147b3ac7c1e7ab40fe6be3612af9468b Mon Sep 17 00:00:00 2001 From: Jason Lowe Date: Fri, 12 Mar 2021 18:52:09 -0600 Subject: [PATCH 3/5] cleanup and use NumberFormatException --- .../scala/com/nvidia/spark/rapids/Plugin.scala | 15 ++++++--------- .../spark/rapids/RapidsExecutorPluginSuite.scala | 1 + 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala index e02b1c37db2..ceaeafc499d 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala @@ -251,21 +251,18 @@ object RapidsExecutorPlugin { * version 7.1.1. */ def cudfVersionSatisfied(expected: String, actual: String): Boolean = { - val expSplits = expected.split('.') - val actSplits = actual.split('.') - val zipSplits = expSplits.zip(actSplits) - // major and minor versions must match exactly - if (zipSplits.take(2).exists{ case (e, a) => e != a }) { + val (majorMinor, patches) = expected.split('.').zip(actual.split('.')).splitAt(2) + if (majorMinor.exists{ case (e, a) => e != a }) { return false } // patch and sub-patch versions can match as long as actual is more recent - zipSplits.drop(2).forall { case (expStr, actStr) => - val expVal = Integer.parseInt(expStr) + patches.forall { case (expStr, actStr) => + val expVal = expStr.toInt try { - expVal <= Integer.parseInt(actStr) + expVal <= actStr.toInt } catch { - case _: IllegalArgumentException => false + case _: NumberFormatException => false } } } diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala index e2731dba897..4d20d38ae21 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala @@ -35,5 +35,6 @@ class RapidsExecutorPluginSuite extends FunSuite { assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.0")) assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.1")) assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.1.1")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.1-special")) } } From 54fec1a98bf7e115329de60ae1032c2613677b74 Mon Sep 17 00:00:00 2001 From: Jason Lowe Date: Mon, 15 Mar 2021 13:51:28 -0500 Subject: [PATCH 4/5] Cleanup checking with Try and startsWith --- .../com/nvidia/spark/rapids/Plugin.scala | 21 +++++++------------ .../rapids/RapidsExecutorPluginSuite.scala | 4 ++++ 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala index ceaeafc499d..92da5f807d1 100644 --- a/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala +++ b/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala @@ -21,6 +21,7 @@ import java.util.Properties import java.util.concurrent.atomic.{AtomicBoolean, AtomicReference} import scala.collection.JavaConverters._ +import scala.util.Try import com.nvidia.spark.rapids.python.PythonWorkerSemaphore @@ -251,19 +252,13 @@ object RapidsExecutorPlugin { * version 7.1.1. */ def cudfVersionSatisfied(expected: String, actual: String): Boolean = { - val (majorMinor, patches) = expected.split('.').zip(actual.split('.')).splitAt(2) - if (majorMinor.exists{ case (e, a) => e != a }) { - return false - } - - // patch and sub-patch versions can match as long as actual is more recent - patches.forall { case (expStr, actStr) => - val expVal = expStr.toInt - try { - expVal <= actStr.toInt - } catch { - case _: NumberFormatException => false - } + val (expMajorMinor, expPatch) = expected.split('.').splitAt(2) + val (actMajorMinor, actPatch) = actual.split('.').splitAt(2) + actMajorMinor.startsWith(expMajorMinor) && { + val expPatchInts = expPatch.map(_.toInt) + val actPatchInts = actPatch.map(v => Try(v.toInt).getOrElse(Int.MinValue)) + val zipped = expPatchInts.zipAll(actPatchInts, 0, 0) + zipped.forall { case (e, a) => e <= a } } } } diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala index 4d20d38ae21..a60b9981733 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala @@ -28,10 +28,14 @@ class RapidsExecutorPluginSuite extends FunSuite { assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0", "7.0")) assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0", "7.0.1")) assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0", "7.0.1.3")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0", "7")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0", "7.1")) assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.1")) assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.1.3")) assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.2")) assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.2.3")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0")) assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.0")) assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.1")) assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.1.1")) From 28ecf1cb8b266f984f3ffaeaf10f04d4dc8f2108 Mon Sep 17 00:00:00 2001 From: Jason Lowe Date: Mon, 15 Mar 2021 15:11:52 -0500 Subject: [PATCH 5/5] Add tests for many patch levels --- .../com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala b/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala index a60b9981733..1ca5aad2bf4 100644 --- a/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala +++ b/tests/src/test/scala/com/nvidia/spark/rapids/RapidsExecutorPluginSuite.scala @@ -40,5 +40,7 @@ class RapidsExecutorPluginSuite extends FunSuite { assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.1")) assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.1.1")) assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.1", "7.0.1-special")) + assert(!RapidsExecutorPlugin.cudfVersionSatisfied("7.0.2.2.2", "7.0.2.2")) + assert(RapidsExecutorPlugin.cudfVersionSatisfied("7.0.2.2.2", "7.0.2.2.2")) } }