Qualification: fix sorting and add unit-tests script #5869

amahussein · 2022-06-17T21:01:39Z

Signed-off-by: Ahmed Hussein (amahussein) a@ahussein.me

fixes #5859 and fixes #2949

Changes

New: replaced calls to html in the JS which used to cause false triggers of unsafe HTML.
A small change in Qualification.scala
- Instead of sorting EstimatedInfo, then QualInfoSummary separately. Now, the code sorts QualInfoSummary first. Then the estimatedInfo array is extracted. This guarantees that the detailed report and the executive-summary have the same order when the default sorting is picked "Desc".
- break the tie of order using the "endTime" as last resort.
- this required a fix in one of the expectations csv file.
Added a script in tools/src/test/resources/dev/run-qualification-tests.sh
- The new script can be used to regenerate the csv files.
- example usage
```
./run-qualification-tests.sh --cp=$JAVA_CP 
```

The above command will simply iterate on all the expected csv file and re-run the qualification tools. The generated output is then compared to the expected files.


"Run the Qualification tool to generate CSV files to update/replace Qualification expectations set."
"The script requires the following:"
"  1- A static var \$qualification_path_map <string, array<string>> that maps between"
"     unit-test name and the array of event logs to be processed for the test. This map"
"     needs to be updated as new test are added and/or log files of existing tests are"
"     modified."
"  2- \$qual_expectations_dir: A directory that has the expected csv files."
"     For each key K in \$qualification_path_map, a csv file should exist"
"     \${qual_expectations_dir}/\$k.csv"
"  3- \$qual_events_dir: A directory that has the event logs to be processed."

"The script iterates on the unit tests. For each key \"K\" in \$qualification_path_map:"
"  1- It runs the Qualification tool given the specified event logs."
"     The argument qual_out_dir=\${out-dir}/qualification-output/runs-output/\$K"
"     QualificationMain --output-directory \${qual_out_dir} \\"
"                        \${qualification_path_map[\$K]}"
"  2- It copies the rapids_4_spark_qualification_output.csv from \$qual_out_dir/"
"     to a new folder \${out-dir}/qualification-output/csv/\$K.csv"
"  3- Logs the result of \"diff\" between the two CSV files"
"     diff \${qual_expectations_dir}/\$K.csv \${out-dir}/qualification-output/csv/\$K.csv"

"Finally, the script logs the result of \"diff\" for two directories:"
"     diff \${qual_expectations_dir}/ \${out-dir}/qualification-output/csv/"
"The script returns a non-zero value if the two folders do not match."
log_msg
"Please make sure to update the CSV files for the failing unit tests."
log_msg
log_info "Optional Arguments:"
"  --rapids-jars-dir=<arg>       - Directory containing RAPIDS jars. By default the script sets it to the"
"                                  target directory of tools."
"                                  If the tools jar is loaded from default directory tools/target/**,"
"                                  then you may need to append the remaining RAPIDS jars to the \"--cp\""
"                                  argument."
"  --cp=<arg>                    - Classpath required as dependencies to run the qualification tool."
"                                  For example, if \$SPARK_HOME/jars are not in the directory <rapids-jars>,"
"                                  then pass it as --cp=\$SPARK_HOME/jars/*:\$CLASS_PATH"
"  --out-dir=<arg>               - Output directory passed to the qualification tool runs."
"                                  By default is is set to test/resources/dev/qualification-output"
"                                  The directory has two subdirectories: csv/ which has the CSV files; and"
"                                  runs-output/ which has the actual output of the tool."
"                                  target directory of tools."
"  --qual-expectations-dir=<arg> - Path of the reference CSV files."
"                                  By default is is set to test/resources/QualificationExpectations"
"  --qual-events-dir=<arg>       - By default, the scripts looks for log files inside the following two"
"                                  directories:"
"                                      - test/resources/spark-events-qualification; and"
"                                      - test/resources/spark-events-profiling"
"                                  When this argument is set in the CLI, it is the directory path of the Spark"
"                                  events logs used as input for the unit tests. So, the user needs to make"
"                                  sure that this directory has all the log files needed by the unit tests."
"  --prof-log-dir=<arg>          - Some the unit tests reads files located in another directory."
"                                  By default is is set to test/resources/spark-events-profiling"
"  --heap=<arg>                  - optional heap size. Default is 10g."
"  --help|-h                     - Shows Help."

"Example Usage:"
"  run-qualification-tests.sh --cp=\$CLASS_PATH --heap=5g"
"  This is equivalent to:"
"    java -Xmx5g \\"
"         -cp rapids-4-spark-tools_2.12-<version>-SNAPSHOT.jar:\$CLASS_PATH \\"
"         com.nvidia.spark.rapids.tool.qualification.QualificationMain \\"
"         --no-html-report --output-directory file:\$qual_out_dir \\"
"         \$LOGFILES"

"How to Add New Test:"
" 1- add a dummy csv file in \$qual-expectations-dir <new_test_name.csv>"
" 2- add the eventlog into the \$qual-events-dir <new_unit_log>"
" 3- update the definition of the hash in define_qualification_tests_map() to map between"
"    the expected file and the unit test name."
" 4- run the script. It is expected that the script fails because the dummy csv is incorrect."
" 5- copy the content of the generated output \$out-dir/csv/new_unit_test.csv"
"    into the expectation folders."

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

amahussein · 2022-06-17T21:11:50Z

build

amahussein · 2022-06-17T21:24:57Z

build

tgravescs

I need to run the script myself still, this will be very nice to have.

tools/src/test/resources/dev/run-qualification-tests.sh

tgravescs · 2022-06-21T14:11:26Z

tools/src/test/resources/dev/run-qualification-tests.sh

+   log_msg "                                  By default is is set to test/resources/QualificationExpectations"
+   log_msg "  --qual-events-dir=<arg>       - Path of the Spark events logs used as input for the unit tests"
+   log_msg "                                  By default is is set to test/resources/spark-events-qualification"
+   log_msg "  --prof-log-dir=<arg>          - Some the unit tests reads files located in another directory."


nit grammar "Some the". What happens if we just call this twice instead of having a separate option, once on qualification dir and once on files in profiling dir? We don't use all the profiling once I thought so I assume user picks and chooses which ones to copy?

This probably would be the way when the script supports both tools.
For now, having a single directory and calling twice could lead to:

It will be more complicated to compare the final results (comparing entire folder). That final check is necessary to make sure that all unit tests exist and equal.

forget to run twice for different output.

Should we consider duplicating the common spark-log files from spark-events-profiling to spark-events-qualification ? Eventually, it will make things easier.

I got rid of the second directory. Basically, if an input directory is passed to the script, then we should expect that it contains all the required log files.
I also updated the description to emphasize that.

need to remove the option then. sorry if I was confusing, what does that mean then for this script? we don't have all our eventlogs in one directory, so this script won't work with it?

Or you mean by default it works with our test setup, but if someone was using it for something else, they must all be in the same directory?

Or you mean by default it works with our test setup, but if someone was using it for something else, they must all be in the same directory?

Yes, exactly. this is what I meant.
By default, it will work fine for our test setup loading the log files from two directories.

tools/src/test/resources/dev/run-qualification-tests.sh

tgravescs · 2022-06-21T14:16:39Z

tools/src/test/resources/dev/run-qualification-tests.sh

+  fi
+
+  # cleanup output folder before running tests
+  if [ -d "$BATCH_OUT_DIR" ]; then


it would be safer to fail if directory exists unless users adds and overwrite flag

I thought about that, but the problem with giving the user option not to overwrite is that a csv from previous run that was not generated by the most recent run would cause the final directory comparison to pass successfully.

I'm not sure I understand, it would fail if any previous run is there and you try to run it again. If you select overwrite then it removes it first.
I'm ok with leaving it this way, but please add comment in top level documentation.

Good point. I added one more flag "-f|--force"

tools/src/test/resources/dev/run-qualification-tests.sh

tgravescs · 2022-06-21T14:24:53Z

tools/src/test/resources/dev/run-qualification-tests.sh

+  qual_log_prefix="file:${QUAL_LOG_DIR}"
+  prof_log_prefix="file:${PROF_LOG_DIR}"
+
+  qualification_path_map[nds_q86_test_expectation]="${qual_log_prefix}/nds_q86_test ${qual_log_prefix}/malformed_json_eventlog.zstd"


@nartal1 just fyi, lets make sure we update this when adding new files.

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

amahussein · 2022-06-22T22:09:46Z

I made some changes and added an update to the PR description.
The qualification JS code cause some false security checks. The fix put in this PR is to replace .HTML() with a method getter that does not trigger any XSS unsafe checks.

amahussein · 2022-06-22T22:15:20Z

build

tgravescs · 2022-06-23T14:37:23Z

tools/src/test/resources/dev/run-qualification-tests.sh

+   log_msg "  3- Logs the result of \"diff\" between the two CSV files"
+   log_msg "     diff \${qual_expectations_dir}/\$K.csv \${out-dir}/qualification-output/csv/\$K.csv"
+   log_msg
+   log_msg "Finally, the script logs the result of \"diff\" for two directories:"


this is same statement as 3- above

I clarified the difference.
"3-" is diff between two CSV files.

The following step is to compare the entire two directories. I found that this step is necessary in case the Map is missing keys. Comparing the final directory is a second level to validate that the Map is not broken.

tgravescs · 2022-06-23T14:50:22Z

tools/src/test/resources/dev/run-qualification-tests.sh

+  fi
+
+  # cleanup output folder before running tests
+  if [ -d "$BATCH_OUT_DIR" ]; then


I'm not sure I understand, it would fail if any previous run is there and you try to run it again. If you select overwrite then it removes it first.
I'm ok with leaving it this way, but please add comment in top level documentation.

tgravescs · 2022-06-23T14:54:57Z

tools/src/test/resources/dev/run-qualification-tests.sh

+   log_msg "                                  By default is is set to test/resources/QualificationExpectations"
+   log_msg "  --qual-events-dir=<arg>       - Path of the Spark events logs used as input for the unit tests"
+   log_msg "                                  By default is is set to test/resources/spark-events-qualification"
+   log_msg "  --prof-log-dir=<arg>          - Some the unit tests reads files located in another directory."


need to remove the option then. sorry if I was confusing, what does that mean then for this script? we don't have all our eventlogs in one directory, so this script won't work with it?

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

amahussein · 2022-06-23T17:58:43Z

build

amahussein · 2022-06-23T18:12:26Z

build

tgravescs · 2022-06-23T20:04:43Z

tools/src/test/resources/dev/run-qualification-tests.sh

@@ -0,0 +1,451 @@
+#!/usr/bin/env bash


sorry can we update the script name to something like generate-qualification-test-results, otherwise someone might think this is to run the tests

name change

tgravescs · 2022-06-23T20:06:16Z

so one thing we didn't address in #2949, is the documentation, I'm fine with waiting and see if we really need it.

tgravescs · 2022-06-23T20:06:31Z

build

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

amahussein · 2022-06-23T20:35:00Z

build

amahussein · 2022-06-24T01:07:20Z

build

Qualification: fix sorting and add unit-tests script

2f2ec37

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

amahussein added the tools label Jun 17, 2022

amahussein added this to the Jun 20 - Jul 8 milestone Jun 17, 2022

amahussein self-assigned this Jun 17, 2022

amahussein requested a review from nartal1 June 17, 2022 21:12

amahussein requested a review from tgravescs June 17, 2022 21:25

tgravescs reviewed Jun 21, 2022

View reviewed changes

amahussein added 2 commits June 22, 2022 14:00

replace HTML string calls in Qualification js

a7f4b39

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

address PR comments for the script

12454a9

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

tgravescs reviewed Jun 23, 2022

View reviewed changes

add overwrite argument, and verbose

98bd74e

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

tgravescs previously approved these changes Jun 23, 2022

View reviewed changes

tgravescs reviewed Jun 23, 2022

View reviewed changes

rename script generate-qualification-test-results

2ce7998

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

tgravescs approved these changes Jun 24, 2022

View reviewed changes

tgravescs merged commit b768d8c into NVIDIA:branch-22.08 Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qualification: fix sorting and add unit-tests script #5869

Qualification: fix sorting and add unit-tests script #5869

amahussein commented Jun 17, 2022 •

edited

Loading

amahussein commented Jun 17, 2022

amahussein commented Jun 17, 2022

tgravescs left a comment

tgravescs Jun 21, 2022

amahussein Jun 22, 2022

amahussein Jun 22, 2022

tgravescs Jun 23, 2022

tgravescs Jun 23, 2022

amahussein Jun 23, 2022

tgravescs Jun 21, 2022

amahussein Jun 22, 2022

tgravescs Jun 23, 2022

amahussein Jun 23, 2022

tgravescs Jun 21, 2022

amahussein commented Jun 22, 2022 •

edited

Loading

amahussein commented Jun 22, 2022

tgravescs Jun 23, 2022

amahussein Jun 23, 2022

tgravescs Jun 23, 2022

tgravescs Jun 23, 2022

amahussein commented Jun 23, 2022

amahussein commented Jun 23, 2022

tgravescs Jun 23, 2022

tgravescs commented Jun 23, 2022

tgravescs commented Jun 23, 2022

amahussein commented Jun 23, 2022

amahussein commented Jun 24, 2022

Qualification: fix sorting and add unit-tests script #5869

Qualification: fix sorting and add unit-tests script #5869

Conversation

amahussein commented Jun 17, 2022 • edited Loading

amahussein commented Jun 17, 2022

amahussein commented Jun 17, 2022

tgravescs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amahussein commented Jun 22, 2022 • edited Loading

amahussein commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amahussein commented Jun 23, 2022

amahussein commented Jun 23, 2022

Choose a reason for hiding this comment

tgravescs commented Jun 23, 2022

tgravescs commented Jun 23, 2022

amahussein commented Jun 23, 2022

amahussein commented Jun 24, 2022

amahussein commented Jun 17, 2022 •

edited

Loading

amahussein commented Jun 22, 2022 •

edited

Loading