You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "wbo4958 (via GitHub)" <gi...@apache.org> on 2024/01/15 09:00:08 UTC
[PR] [CORE][TESTS] make gpu fraction tests more robust [spark]
wbo4958 opened a new pull request, #44735:
URL: https://github.com/apache/spark/pull/44735
### What changes were proposed in this pull request?
When cherry-picking https://github.com/apache/spark/pull/43494 back to branch 3.5 https://github.com/apache/spark/pull/44690,
I ran into the issue that some tests for Scala 2.12 failed when comparing two maps. It turned out that the function [compareMaps](https://github.com/apache/spark/pull/43494/files#diff-f205431247dd9446f4ce941e5a4620af438c242b9bdff6e7faa7df0194db49acR129) is not so robust for scala 2.12 and scala 2.13.
- scala 2.13
``` scala
Welcome to Scala 2.13.12 (OpenJDK 64-Bit Server VM, Java 17.0.9).
Type in expressions for evaluation. Or try :help.
scala> def compareMaps(lhs: Map[String, Double], rhs: Map[String, Double],
| eps: Double = 0.00000001): Boolean = {
| lhs.size == rhs.size &&
| lhs.zip(rhs).forall { case ((lName, lAmount), (rName, rAmount)) =>
| lName == rName && (lAmount - rAmount).abs < eps
| }
| }
|
| import scala.collection.mutable.HashMap
| val resources = Map("gpu" -> Map("a" -> 1.0, "b" -> 2.0, "c" -> 3.0, "d"-> 4.0))
| val mapped = resources.map { case (rName, addressAmounts) =>
| rName -> HashMap(addressAmounts.toSeq.sorted: _*)
| }
|
| compareMaps(resources("gpu"), mapped("gpu").toMap)
def compareMaps(lhs: Map[String,Double], rhs: Map[String,Double], eps: Double): Boolean
import scala.collection.mutable.HashMap
val resources: scala.collection.immutable.Map[String,scala.collection.immutable.Map[String,Double]] = Map(gpu -> Map(a -> 1.0, b -> 2.0, c -> 3.0, d -> 4.0))
val mapped: scala.collection.immutable.Map[String,scala.collection.mutable.HashMap[String,Double]] = Map(gpu -> HashMap(a -> 1.0, b -> 2.0, c -> 3.0, d -> 4.0))
val res0: Boolean = true
```
- scala 2.12
``` scala
Welcome to Scala 2.12.14 (OpenJDK 64-Bit Server VM, Java 17.0.9).
Type in expressions for evaluation. Or try :help.
scala> def compareMaps(lhs: Map[String, Double], rhs: Map[String, Double],
| eps: Double = 0.00000001): Boolean = {
| lhs.size == rhs.size &&
| lhs.zip(rhs).forall { case ((lName, lAmount), (rName, rAmount)) =>
| lName == rName && (lAmount - rAmount).abs < eps
| }
| }
compareMaps: (lhs: Map[String,Double], rhs: Map[String,Double], eps: Double)Boolean
scala> import scala.collection.mutable.HashMap
import scala.collection.mutable.HashMap
scala> val resources = Map("gpu" -> Map("a" -> 1.0, "b" -> 2.0, "c" -> 3.0, "d"-> 4.0))
resources: scala.collection.immutable.Map[String,scala.collection.immutable.Map[String,Double]] = Map(gpu -> Map(a -> 1.0, b -> 2.0, c -> 3.0, d -> 4.0))
scala> val mapped = resources.map { case (rName, addressAmounts) =>
| rName -> HashMap(addressAmounts.toSeq.sorted: _*)
| }
mapped: scala.collection.immutable.Map[String,scala.collection.mutable.HashMap[String,Double]] = Map(gpu -> Map(b -> 2.0, d -> 4.0, a -> 1.0, c -> 3.0))
scala> compareMaps(resources("gpu"), mapped("gpu").toMap)
res0: Boolean = false
```
The same code bug got different results for Scala 2.12 and Scala 2.13. This PR tried to rework compareMaps to make tests pass for both scala 2.12 and scala 2.13
### Why are the changes needed?
Some users may back-port https://github.com/apache/spark/pull/43494 to some older branch for scala 2.12 and will run into the same issue. It's just trivial work to make the GPU fraction tests compatible with Scala 2.12 and Scala 2.13
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Make sure all the CI pipelines pass
### Was this patch authored or co-authored using generative AI tooling?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46721][CORE][TESTS] Make gpu fraction tests more robust [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 closed pull request #44735: [SPARK-46721][CORE][TESTS] Make gpu fraction tests more robust
URL: https://github.com/apache/spark/pull/44735
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46721][CORE][TESTS] make gpu fraction tests more robust [spark]
Posted by "wbo4958 (via GitHub)" <gi...@apache.org>.
wbo4958 commented on PR #44735:
URL: https://github.com/apache/spark/pull/44735#issuecomment-1891624833
Hi @tgravescs, Could you help to review it? Thx very much.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46721][CORE][TESTS] make gpu fraction tests more robust [spark]
Posted by "wbo4958 (via GitHub)" <gi...@apache.org>.
wbo4958 commented on code in PR #44735:
URL: https://github.com/apache/spark/pull/44735#discussion_r1467182683
##########
core/src/test/scala/org/apache/spark/scheduler/ExecutorResourceUtils.scala:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.spark.scheduler
+
+trait ExecutorResourceUtils {
+
+ def compareMaps(lhsMap: Map[String, Double], rhsMap: Map[String, Double],
+ eps: Double = 0.00000001): Boolean = {
+ val lhs = lhsMap.toSeq.sorted
+ val rhs = rhsMap.toSeq.sorted
+ lhs.size == rhs.size &&
+ lhs.zip(rhs).forall { case ((lName, lAmount), (rName, rAmount)) =>
+ lName == rName && (lAmount - rAmount).abs < eps
+ }
Review Comment:
Cool. Thx. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46721][CORE][TESTS] make gpu fraction tests more robust [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #44735:
URL: https://github.com/apache/spark/pull/44735#discussion_r1466376646
##########
core/src/test/scala/org/apache/spark/scheduler/ExecutorResourceUtils.scala:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.spark.scheduler
+
+trait ExecutorResourceUtils {
+
+ def compareMaps(lhsMap: Map[String, Double], rhsMap: Map[String, Double],
+ eps: Double = 0.00000001): Boolean = {
+ val lhs = lhsMap.toSeq.sorted
+ val rhs = rhsMap.toSeq.sorted
+ lhs.size == rhs.size &&
+ lhs.zip(rhs).forall { case ((lName, lAmount), (rName, rAmount)) =>
+ lName == rName && (lAmount - rAmount).abs < eps
+ }
Review Comment:
How about:
```scala
lhs.size == rhs.size && lhs.forall { case (lName, lAmount) =>
rhs.get(lName).exists(rAmount => (lAmount - rAmount).abs < eps)
}
```
we can avoid sort this way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46721][CORE][TESTS] Make gpu fraction tests more robust [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on PR #44735:
URL: https://github.com/apache/spark/pull/44735#issuecomment-1914076948
Thanks, merged to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org