You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/04/29 07:15:46 UTC
[GitHub] [spark] obs0811 opened a new pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
obs0811 opened a new pull request #32394:
URL: https://github.com/apache/spark/pull/32394
### What changes were proposed in this pull request?
This PR fixes an error in `BenchmarkBase.scala` that occurs when creating a benchmark file in a non-existent directory.
### Why are the changes needed?
When submitting a benchmark job using `org.apache.spark.benchmark.Benchmarks` class with `SPARK_GENERATE_BENCHMARK_FILES=1` option, an exception is raised if the directory where the benchmark file will be generated does not exist.
For more information, please refer to [SPARK-35266](https://issues.apache.org/jira/browse/SPARK-35266).
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
After building Spark, manually tested with the following command:
```
SPARK_GENERATE_BENCHMARK_FILES=1 bin/spark-submit --class \
org.apache.spark.benchmark.Benchmarks --jars \
"`find . -name '*-SNAPSHOT-tests.jar' -o -name '*avro*-SNAPSHOT.jar' | paste -sd ',' -`" \
"`find . -name 'spark-core*-SNAPSHOT-tests.jar'`" \
"org.apache.spark.ml.linalg.BLASBenchmark"
```
It successfully generated the benchmark result files.
**Why it is sufficient:**
As illustrated in the comments in `Benchmarks.scala`, the command below runs all benchmarks and generates the results:
```
SPARK_GENERATE_BENCHMARK_FILES=1 bin/spark-submit --class \
org.apache.spark.benchmark.Benchmarks --jars \
"`find . -name '*-SNAPSHOT-tests.jar' -o -name '*avro*-SNAPSHOT.jar' | paste -sd ',' -`" \
"`find . -name 'spark-core*-SNAPSHOT-tests.jar'`" \
"*"
```
Of all the benchmarks (55 benchmarks in total), only `BLASBenchmark` fails due to the proposed issue for the current code in the master branch. Thus, it is currently sufficient to test `BLASBenchmark` to validate this change.
--
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.
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
[GitHub] [spark] HyukjinKwon edited a comment on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon edited a comment on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-831035088
@srowen and @zhengruifeng FYI from https://github.com/apache/spark/commit/9244066ca69a3fb5d7fe446ce0e19d108892c49d and https://github.com/apache/spark/commit/5b77ebb57ba6edf1f8f4f8e83abef4f9f1ae6175. I think it was perfectly fine without including benchmark results (but codes only) because It was a bit weird to upload the results based on different spec machines.
Now there have been some latest changes at #32015 and #32044, and now the PR authors can run the benchmarks in similar specification very easily (https://spark.apache.org/developer-tools.html#github-workflow-benchmarks), and it makes more sense to include benchmark results in a PR :).
--
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.
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
[GitHub] [spark] HyukjinKwon commented on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-831035088
@srowen and @zhengruifeng FYI from https://github.com/apache/spark/commit/9244066ca69a3fb5d7fe446ce0e19d108892c49d and https://github.com/apache/spark/commit/5b77ebb57ba6edf1f8f4f8e83abef4f9f1ae6175. I think it was perfectly fine without including benchmark results (but codes only) because It was a bit weird to upload the results based on different spec machines.
Now there have been some latest changes at #32015 and #32044, and now the PR authors can run the benchmark results in similar specification very easily (https://spark.apache.org/developer-tools.html#github-workflow-benchmarks), and it makes more sense to include benchmark results in a PR :).
--
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.
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
[GitHub] [spark] HyukjinKwon commented on a change in pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #32394:
URL: https://github.com/apache/spark/pull/32394#discussion_r624883141
##########
File path: core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala
##########
@@ -49,7 +49,11 @@ abstract class BenchmarkBase {
val resultFileName =
s"${this.getClass.getSimpleName.replace("$", "")}$jdkString$suffix-results.txt"
val prefix = Benchmarks.currentProjectRoot.map(_ + "/").getOrElse("")
- val file = new File(s"${prefix}benchmarks/$resultFileName")
+ val dir = new File(s"${prefix}benchmarks/")
+ if (!dir.exists()) {
+ dir.mkdirs()
Review comment:
Can you add `println` and say the directory is going to be created? e.g.)
```scala
// scalastyle:off println
println(s"Creating ${dir.getAbsolutePath} for benchmark results.")
// scalastyle:on println
```
--
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.
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
[GitHub] [spark] HyukjinKwon commented on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-831128765
Thanks for your first contribution and congrats for being a contributor!
--
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.
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
[GitHub] [spark] HyukjinKwon commented on a change in pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #32394:
URL: https://github.com/apache/spark/pull/32394#discussion_r624879992
##########
File path: core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala
##########
@@ -49,7 +49,11 @@ abstract class BenchmarkBase {
val resultFileName =
s"${this.getClass.getSimpleName.replace("$", "")}$jdkString$suffix-results.txt"
val prefix = Benchmarks.currentProjectRoot.map(_ + "/").getOrElse("")
- val file = new File(s"${prefix}benchmarks/$resultFileName")
+ val dir = new File(s"${prefix}benchmarks/")
+ if (!dir.exists()) {
+ dir.mkdirs()
+ }
+ val file = new File(s"${dir}$resultFileName")
Review comment:
Ah, okay. the new benchmark were added at SPARK-33882 and SPARK-35150, that was after https://github.com/apache/spark/pull/32015 and https://github.com/apache/spark/pull/32044.
Okay, @byungsoo-oh, would you mind throwing an exception with `assert` here with a pretty error message? I think the results files should always be existent. I think the results files at SPARK-33882 and SPARK-35150 were not mistakenly added.
--
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.
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
[GitHub] [spark] byungsoo-oh commented on a change in pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
byungsoo-oh commented on a change in pull request #32394:
URL: https://github.com/apache/spark/pull/32394#discussion_r624902959
##########
File path: core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala
##########
@@ -49,7 +49,11 @@ abstract class BenchmarkBase {
val resultFileName =
s"${this.getClass.getSimpleName.replace("$", "")}$jdkString$suffix-results.txt"
val prefix = Benchmarks.currentProjectRoot.map(_ + "/").getOrElse("")
- val file = new File(s"${prefix}benchmarks/$resultFileName")
+ val dir = new File(s"${prefix}benchmarks/")
+ if (!dir.exists()) {
+ dir.mkdirs()
Review comment:
Thanks for the comment :) I added `println` as suggested.
--
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.
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
[GitHub] [spark] HyukjinKwon commented on a change in pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #32394:
URL: https://github.com/apache/spark/pull/32394#discussion_r624881857
##########
File path: core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala
##########
@@ -49,7 +49,11 @@ abstract class BenchmarkBase {
val resultFileName =
s"${this.getClass.getSimpleName.replace("$", "")}$jdkString$suffix-results.txt"
val prefix = Benchmarks.currentProjectRoot.map(_ + "/").getOrElse("")
- val file = new File(s"${prefix}benchmarks/$resultFileName")
+ val dir = new File(s"${prefix}benchmarks/")
+ if (!dir.exists()) {
+ dir.mkdirs()
+ }
+ val file = new File(s"${dir}$resultFileName")
Review comment:
@byungsoo-oh can you add an assert here with a pretty error message saying the `benchmarks` directory has to be manually created? My concern is that it happened to create `benchmarks` directly in a weird location mistakenly (because the directories are based on the jar location which is pretty flaky and hacky)
##########
File path: core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala
##########
@@ -49,7 +49,11 @@ abstract class BenchmarkBase {
val resultFileName =
s"${this.getClass.getSimpleName.replace("$", "")}$jdkString$suffix-results.txt"
val prefix = Benchmarks.currentProjectRoot.map(_ + "/").getOrElse("")
- val file = new File(s"${prefix}benchmarks/$resultFileName")
+ val dir = new File(s"${prefix}benchmarks/")
+ if (!dir.exists()) {
+ dir.mkdirs()
+ }
+ val file = new File(s"${dir}$resultFileName")
Review comment:
@byungsoo-oh can you instead add an assert here with a pretty error message saying the `benchmarks` directory has to be manually created? My concern is that it happened to create `benchmarks` directly in a weird location mistakenly (because the directories are based on the jar location which is pretty flaky and hacky)
--
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.
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
[GitHub] [spark] HyukjinKwon closed pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #32394:
URL: https://github.com/apache/spark/pull/32394
--
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-829008876
Can one of the admins verify this patch?
--
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.
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
[GitHub] [spark] HyukjinKwon commented on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-831032690
@byungsoo-oh:
1. would you mind checking https://github.com/apache/spark/pull/32394/checks?check_run_id=2464430892 and enable GitHub Actions in your forked repository?
2. It would be great if you're interested in submitting another PR to generate and update the results added at SPARK-33882 and SPARK-35150 (after this PR is merge). It's pretty straightforward to generate them: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks
--
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.
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
[GitHub] [spark] HyukjinKwon commented on a change in pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #32394:
URL: https://github.com/apache/spark/pull/32394#discussion_r624880366
##########
File path: core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala
##########
@@ -49,7 +49,11 @@ abstract class BenchmarkBase {
val resultFileName =
s"${this.getClass.getSimpleName.replace("$", "")}$jdkString$suffix-results.txt"
val prefix = Benchmarks.currentProjectRoot.map(_ + "/").getOrElse("")
- val file = new File(s"${prefix}benchmarks/$resultFileName")
+ val dir = new File(s"${prefix}benchmarks/")
+ if (!dir.exists()) {
+ dir.mkdirs()
+ }
+ val file = new File(s"${dir}$resultFileName")
Review comment:
Ah, okay. the new benchmark were added at SPARK-33882 and SPARK-35150, that was after https://github.com/apache/spark/pull/32015 and https://github.com/apache/spark/pull/32044.
--
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.
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
[GitHub] [spark] HyukjinKwon commented on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-831035815
ok to test
--
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.
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
[GitHub] [spark] HyukjinKwon commented on a change in pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #32394:
URL: https://github.com/apache/spark/pull/32394#discussion_r624883369
##########
File path: core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala
##########
@@ -49,7 +49,11 @@ abstract class BenchmarkBase {
val resultFileName =
s"${this.getClass.getSimpleName.replace("$", "")}$jdkString$suffix-results.txt"
val prefix = Benchmarks.currentProjectRoot.map(_ + "/").getOrElse("")
- val file = new File(s"${prefix}benchmarks/$resultFileName")
+ val dir = new File(s"${prefix}benchmarks/")
+ if (!dir.exists()) {
+ dir.mkdirs()
Review comment:
My concern is that the benchmark directory is based on jars paths which are flaky. Might be better to explicitly show.
--
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.
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
[GitHub] [spark] HyukjinKwon commented on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-831032796
ok to test
--
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.
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
[GitHub] [spark] HyukjinKwon commented on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-831128152
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.
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
[GitHub] [spark] byungsoo-oh commented on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
byungsoo-oh commented on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-831057105
> @byungsoo-oh:
>
> 1. would you mind checking https://github.com/apache/spark/pull/32394/checks?check_run_id=2464430892 and enable GitHub Actions in your forked repository?
> 2. It would be great if you're interested in submitting another PR to generate and update the results added at [SPARK-33882](https://issues.apache.org/jira/browse/SPARK-33882) and [SPARK-35150](https://issues.apache.org/jira/browse/SPARK-35150) (after this PR is merge). It's pretty straightforward to generate them: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks
@HyukjinKwon
Thank you for the comments :)
1. I enabled it in my forked repository.
2. I would be delighted if I could submit another PR to add the benchmark results.
--
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.
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
[GitHub] [spark] byungsoo-oh commented on a change in pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
byungsoo-oh commented on a change in pull request #32394:
URL: https://github.com/apache/spark/pull/32394#discussion_r624944873
##########
File path: core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala
##########
@@ -49,7 +49,11 @@ abstract class BenchmarkBase {
val resultFileName =
s"${this.getClass.getSimpleName.replace("$", "")}$jdkString$suffix-results.txt"
val prefix = Benchmarks.currentProjectRoot.map(_ + "/").getOrElse("")
- val file = new File(s"${prefix}benchmarks/$resultFileName")
+ val dir = new File(s"${prefix}benchmarks/")
+ if (!dir.exists()) {
+ dir.mkdirs()
+ }
+ val file = new File(s"${dir}$resultFileName")
Review comment:
I got the point. Thank you!
--
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #32394: [SPARK-35266][TESTS] Fix error in BenchmarkBase.scala that occurs when creating benchmark files in non-existent directory
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32394:
URL: https://github.com/apache/spark/pull/32394#issuecomment-829008876
Can one of the admins verify this patch?
--
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.
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