Skip to content

Conversation

@wbo4958
Copy link
Contributor

@wbo4958 wbo4958 commented Jan 15, 2024

What changes were proposed in this pull request?

When cherry-picking #43494 back to branch 3.5 #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 is not so robust for scala 2.12 and scala 2.13.

  • scala 2.13
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
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 #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

@github-actions github-actions bot added the CORE label Jan 15, 2024
@wbo4958 wbo4958 changed the title [CORE][TESTS] make gpu fraction tests more robust [SPARK-46721][CORE][TESTS] make gpu fraction tests more robust Jan 15, 2024
@wbo4958
Copy link
Contributor Author

wbo4958 commented Jan 15, 2024

Hi @tgravescs, Could you help to review it? Thx very much.

Comment on lines 26 to 29
lhs.size == rhs.size &&
lhs.zip(rhs).forall { case ((lName, lAmount), (rName, rAmount)) =>
lName == rName && (lAmount - rAmount).abs < eps
}
Copy link
Member

Choose a reason for hiding this comment

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

How about:

lhs.size == rhs.size && lhs.forall { case (lName, lAmount) =>
  rhs.get(lName).exists(rAmount => (lAmount - rAmount).abs < eps)
}

we can avoid sort this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Thx. Done.

Copy link
Member

@Ngone51 Ngone51 left a comment

Choose a reason for hiding this comment

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

LGTM

@Ngone51 Ngone51 changed the title [SPARK-46721][CORE][TESTS] make gpu fraction tests more robust [SPARK-46721][CORE][TESTS] Make gpu fraction tests more robust Jan 29, 2024
@Ngone51 Ngone51 closed this in 90e6c0c Jan 29, 2024
@Ngone51
Copy link
Member

Ngone51 commented Jan 29, 2024

Thanks, merged to master!

@wbo4958 wbo4958 deleted the gpu-fraction-tests branch January 29, 2024 07:01
wbo4958 added a commit to wbo4958/spark that referenced this pull request Jan 30, 2024
When cherry-picking apache#43494 back to branch 3.5 apache#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

Some users may back-port apache#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

No

Make sure all the CI pipelines pass

No

Closes apache#44735 from wbo4958/gpu-fraction-tests.

Authored-by: Bobby Wang <[email protected]>
Signed-off-by: Yi Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants