Fix the mismatching default configs in integration tests #11283

jihoonson · 2024-08-01T01:16:44Z

Default values of some configs used in the integration test by default has gone stale as they haven't updated when their defaults changed. This is because we have a separated list of default configs used by the integration test, which should be manually updated along with code change. This PR fixes this problem by dumping all default configs in a file and letting the integration test use it. A new interface, getDefault, has been added in ConfEntry. A new DumpDefaultConfigs class uses this API to dump all default configs in a file. In the integration test, those default configs are loaded when the spark session is initialized.

integration_tests/pom.xml

integration_tests/run_pyspark_from_build.sh

integration_tests/src/main/python/spark_session.py

integration_tests/src/main/scala/com/nvidia/spark/rapids/tests/DumpDefaultConfigs.scala

gerashegalov · 2024-08-01T05:59:44Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConf.scala

@@ -124,6 +124,7 @@ abstract class ConfEntry[T](val key: String, val converter: String => T, val doc

  def get(conf: Map[String, String]): T
  def get(conf: SQLConf): T
+  def getDefault(): T


It would rather make sense to omit the conf withtout defaults in the dump IMO

So, I think this API should just return whatever the default is. Any business logic should not be involved at this level.

As for DumpDefaultConfigs, it dumps even those configs without defaults because we would want to unset them for the integration test if they were set somehow.

…a new stage for integration test to populate default configs Signed-off-by: Jihoon Son <ghoonson@gmail.com>

jihoonson · 2024-08-01T18:19:21Z

@@ -1439,7 +1439,7 @@ This will force full Scala code rebuild in downstream modules.
                 <plugin>
                     <groupId>org.codehaus.mojo</groupId>
                     <artifactId>exec-maven-plugin</artifactId>
-                    <version>3.0.0</version>
+                    <version>3.3.0</version>
                 </plugin>

Uh, the CI fails with this error. What should I do about it? I bumped up this dependency version because the provided classpathScope has been introduced in that version.

jihoonson · 2024-08-01T20:37:04Z

The CI error seems because of the missing version update in the 2.13 pom file for the corresponding dependency. I will fix it soon.

jlowe

Since this is adding a new dependency to the integration tests, it needs to update integration_tests/src/assembly/bin.xml to add the file to the integration tests tarball so CI test pipelines that do not build from source can run the integration tests properly.

integration_tests/run_pyspark_from_build.sh

jihoonson · 2024-08-01T21:59:28Z

@jlowe thanks for catching it. I added the new file in 8dbd354, but I'm not sure if I did it right. I opened up the integration_tests/target/rapids-4-spark-integration-tests_2.12-24.08.0-SNAPSHOT-sparkxxx-jar-with-dependencies.jar, but I don't see it there. Should the file be added somewhere else than this jar file? In fact, I don't seem to see any of other files as well in the integration_tests/src/assembly/bin.xml file.

integration_tests/src/assembly/bin.xml

jihoonson · 2024-08-01T22:36:46Z

java.lang.reflect.InaccessibleObjectException: Unable to make protected void java.net.URLClassLoader.addURL(java.net.URL) accessible: module java.base does not "opens java.net" to unnamed module @6808428c
    at java.lang.reflect.AccessibleObject.checkCanSetAccessible (AccessibleObject.java:354)
    at java.lang.reflect.AccessibleObject.checkCanSetAccessible (AccessibleObject.java:297)
    at java.lang.reflect.Method.checkCanSetAccessible (Method.java:200)
    at java.lang.reflect.Method.setAccessible (Method.java:194)
    at org.apache.commons.lang3.reflect.MethodUtils.invokeMethod (MethodUtils.java:839)
    at org.apache.commons.lang3.reflect.MethodUtils.invokeMethod (MethodUtils.java:802)
    at com.nvidia.spark.rapids.ShimLoader$.$anonfun$updateSparkClassLoader$2 (ShimLoader.scala:165)
    at com.nvidia.spark.rapids.ShimLoader$.$anonfun$updateSparkClassLoader$2$adapted (ShimLoader.scala:161)
    at scala.collection.immutable.List.foreach (List.scala:334)
    at com.nvidia.spark.rapids.ShimLoader$.$anonfun$updateSparkClassLoader$1 (ShimLoader.scala:161)
    at com.nvidia.spark.rapids.ShimLoader$.$anonfun$updateSparkClassLoader$1$adapted (ShimLoader.scala:160)
    at scala.Option.foreach (Option.scala:437)
    at com.nvidia.spark.rapids.ShimLoader$.updateSparkClassLoader (ShimLoader.scala:160)
    at com.nvidia.spark.rapids.ShimLoader$.getShimClassLoader (ShimLoader.scala:186)
    at com.nvidia.spark.rapids.ShimReflectionUtils$.loadClass (ShimReflectionUtils.scala:28)
    at com.nvidia.spark.rapids.tests.DumpDefaultConfigs$.main (DumpDefaultConfigs.scala:50)

Hmm so the CI is now failing with the error above. I think it's because of some missing JVM options, such as --add-opens=java.base/java.net=ALL-UNNAMED for java 17. I'm working on a fix.

integration_tests/pom.xml

integration_tests/run_pyspark_from_build.sh

integration_tests/src/assembly/bin.xml

integration_tests/pom.xml

jihoonson · 2024-08-02T18:22:43Z

As for the CI failure with java 17, to allow access to protected methods via reflection, I have two ideas:

We can add JVM options in MAVEN_OPTS to allow it. This MAVEN_OPTS can be set in the mvn-verify-check.yml so that the github action picks it up. We should update our doc as well to explain that those JVM options should be set to make a build with Java 17.
Instead of letting the maven process run DumpDefaultConfigs, we could spawn a new process by setting the goal of exec-maven-plugin to exec that executes a scala program. This will require making a jar with all the dependencies DumpDefaultConfigs needs, which seems only scopt currently besides the provided rapids-4-spark_2.12 dependency.

I'm leaning towards to the option 1 only because it is easier. But the option 2 doesn't seem that hard either. Any opinions here?

jlowe · 2024-08-02T18:33:00Z

Any opinions here?

We already have an example of running a class as a program with all of its dependencies to generate files during the build which isn't doing any reflection shenanigans and IIUC is working with the JDK17 build. See the update_config_docs rule in dist/pom.xml. Any reason we can't do the same here?

gerashegalov · 2024-08-02T18:43:54Z

+1 for following the pattern from update_config_docs. exec plugin executions are run in the Maven JVM, and requiring particular MAVEN_OPTS is unnecessarily user-unfriendly. In Ant java task https://ant.apache.org/manual/Tasks/java.html we can set fork=true and pass the same args we already use for unit tests ${extraJavaTestArgs} as jvmargs if necessary

jihoonson · 2024-08-08T20:43:35Z

Build

jihoonson · 2024-08-08T21:02:46Z

Can someone help to run the CI? I don't seem to have enough power for that.

gerashegalov · 2024-08-08T23:27:25Z

Can someone help to run the CI? I don't seem to have enough power for that.

Create a PR to add your GH user here

spark-rapids/.github/workflows/blossom-ci.yml

Line 78 in 8ef2e41

github.actor == 'SurajAralihalli'

gerashegalov · 2024-08-08T23:27:30Z

build

jihoonson · 2024-08-09T18:05:58Z

The CI is failing because some tests in hash_aggregate_test are assuming spark.sql.legacy.allowNegativeScaleOfDecimal to be set to false by default, which is good as this is exactly what we want to catch and fix with this PR 🙂 Though, those tests (test_hash_grpby_sum and test_hash_grpby_sum_full_decimal) seem to intend to test certain cases with decimals with negative scales, so I think we should keep the negative decimal scale enabled for these tests. In c4ba1a2, I added conf = copy_and_update(conf, {'spark.sql.legacy.allowNegativeScaleOfDecimal': 'true'}) to overwrite this property in each test. I'm not quite sure if this is the best way to do so though, please let me know.

gerashegalov

LGTM with one caveat:

Just double-checking:
beyond propagating the classpath, part of the reason why we looked into forking out a JVM for config dump execution is to propagate extra options for JDK17. #11283 (comment)

gerashegalov · 2024-08-09T18:23:35Z

integration_tests/pom.xml

+                            <target>
+                                <taskdef resource="net/sf/antcontrib/antcontrib.properties"/>
+                                <java classname="com.nvidia.spark.rapids.tests.DumpDefaultConfigs"
+                                    failonerror="true"
+                                    classpathref="maven.compile.classpath"
+                                    fork="true">
+                                    <arg value="json"/>
+                                    <arg value="${project.build.directory}/spark-rapids-default-configs.json"/>
+                                </java>
+                            </target>


Curious why we end up not needing this #11283 (comment)

Good question. I'm not quite sure why it works. I can add those java options here just in case..

jihoonson · 2024-08-09T20:40:52Z

It is necessary to set fork=true, if the class being launched by this task or any libraries being used by that class, call APIs like java.lang.System.exit() or java.lang.Runtime.exit().

I have noticed that the RapidsConf task in dist/pom.xml should set fork=true as well per https://ant.apache.org/manual/Tasks/java.html since now it can call System.exit(). Fixed this in 45beca3.

jihoonson · 2024-08-09T21:10:25Z

I have noticed that the RapidsConf task in dist/pom.xml should set fork=true as well per https://ant.apache.org/manual/Tasks/java.html since now it can call System.exit(). Fixed this in 45beca3.

NVM. Java 8 seems to fail to have some classpath issue with this change. It could be nice to fix, but I don't think the change that adds System.exit() in RapidsConf.main() is necessary for this PR, so I just reverted it.

jihoonson · 2024-08-09T22:30:23Z

build

jihoonson · 2024-08-13T21:56:35Z

build

jihoonson · 2024-08-14T20:14:26Z

Hi @jlowe, I have fixed all tests and the CI is green now. Could you have another look?

integration_tests/src/main/python/arithmetic_ops_test.py

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConf.scala

jihoonson · 2024-08-15T18:21:47Z

build

…DIA#11283)" This reverts commit bc8c577. Signed-off-by: Jihoon Son <ghoonson@gmail.com>

)" (#11347) This reverts commit bc8c577. Signed-off-by: Jihoon Son <ghoonson@gmail.com>

jihoonson requested review from jlowe, revans2, tgravescs, GaryShen2008, NvTimLiu and gerashegalov as code owners August 1, 2024 01:16

gerashegalov reviewed Aug 1, 2024

View reviewed changes

jihoonson added 2 commits August 1, 2024 10:33

Add a new interface to retrieve all configs with their defaults; Add …

e3d6bff

…a new stage for integration test to populate default configs Signed-off-by: Jihoon Son <ghoonson@gmail.com>

address comments

e7f1ec5

jihoonson force-pushed the fix-it-test-defaults branch from 8460739 to e7f1ec5 Compare August 1, 2024 18:00

missing version update in 2.13 pom

37c0f92

jihoonson added 2 commits August 1, 2024 13:39

fix match arms

85f05f3

take the json file path as an input

6ba7300

jlowe reviewed Aug 1, 2024

View reviewed changes

integration_tests/run_pyspark_from_build.sh Outdated Show resolved Hide resolved

jihoonson added 2 commits August 1, 2024 14:54

add the new config file in the assembly

8dbd354

missing 2.13 change

cb88561

jlowe reviewed Aug 1, 2024

View reviewed changes

integration_tests/src/assembly/bin.xml Outdated Show resolved Hide resolved

gerashegalov reviewed Aug 2, 2024

View reviewed changes

integration_tests/pom.xml Outdated Show resolved Hide resolved

integration_tests/run_pyspark_from_build.sh Outdated Show resolved Hide resolved

integration_tests/src/assembly/bin.xml Outdated Show resolved Hide resolved

gerashegalov reviewed Aug 2, 2024

View reviewed changes

integration_tests/pom.xml Outdated Show resolved Hide resolved

jihoonson added 3 commits August 2, 2024 10:04

use maven build directory var

3a26d15

revert unintended change

5d59de7

remove unnecessary clean

a7a04d0

gerashegalov assigned jihoonson Aug 2, 2024

jihoonson dismissed jlowe’s stale review via f336839 August 8, 2024 17:53

jihoonson added the bug Something isn't working label Aug 8, 2024

allow legacy negative scale for decimals for some tests

c4ba1a2

jlowe previously approved these changes Aug 9, 2024

View reviewed changes

gerashegalov previously approved these changes Aug 9, 2024

View reviewed changes

should fork for RapidsConf

45beca3

jihoonson dismissed stale reviews from gerashegalov and jlowe via 45beca3 August 9, 2024 20:39

remove System.exit() from RapidsConf.main()

d45dd1e

missing change for scala 2.13

951c5f3

Fix more tests to set configs

ab5d8b4

jlowe reviewed Aug 15, 2024

View reviewed changes

integration_tests/src/main/python/arithmetic_ops_test.py Show resolved Hide resolved

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConf.scala Outdated Show resolved Hide resolved

add back explicit configs

04d86b8

jlowe approved these changes Aug 15, 2024

View reviewed changes

jihoonson merged commit bc8c577 into NVIDIA:branch-24.10 Aug 15, 2024
43 checks passed

zpuller mentioned this pull request Aug 16, 2024

[BUG] databrick nightly failing with not able to get spark-version-info.properties #11346

Closed

jihoonson added a commit to jihoonson/spark-rapids that referenced this pull request Aug 16, 2024

Revert "Fix the mismatching default configs in integration tests (NVI…

16729e4

…DIA#11283)" This reverts commit bc8c577. Signed-off-by: Jihoon Son <ghoonson@gmail.com>

jihoonson mentioned this pull request Aug 16, 2024

Revert "Fix the mismatching default configs in integration tests (#11283)" #11347

Merged

zpuller pushed a commit that referenced this pull request Aug 17, 2024

Revert "Fix the mismatching default configs in integration tests (#11283

a61b834

)" (#11347) This reverts commit bc8c577. Signed-off-by: Jihoon Son <ghoonson@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the mismatching default configs in integration tests #11283

Fix the mismatching default configs in integration tests #11283

jihoonson commented Aug 1, 2024

gerashegalov Aug 1, 2024

jihoonson Aug 1, 2024 •

edited

Loading

jihoonson commented Aug 1, 2024 •

edited

Loading

jihoonson commented Aug 1, 2024 •

edited

Loading

jlowe left a comment

jihoonson commented Aug 1, 2024 •

edited

Loading

jihoonson commented Aug 1, 2024

jihoonson commented Aug 2, 2024

jlowe commented Aug 2, 2024

gerashegalov commented Aug 2, 2024 •

edited

Loading

jihoonson commented Aug 8, 2024

jihoonson commented Aug 8, 2024

gerashegalov commented Aug 8, 2024

gerashegalov commented Aug 8, 2024

jihoonson commented Aug 9, 2024

gerashegalov left a comment

gerashegalov Aug 9, 2024

jihoonson Aug 9, 2024

jihoonson commented Aug 9, 2024

jihoonson commented Aug 9, 2024

jihoonson commented Aug 9, 2024

jihoonson commented Aug 13, 2024

jihoonson commented Aug 14, 2024

jihoonson commented Aug 15, 2024

Fix the mismatching default configs in integration tests #11283

Fix the mismatching default configs in integration tests #11283

Conversation

jihoonson commented Aug 1, 2024

gerashegalov Aug 1, 2024

Choose a reason for hiding this comment

jihoonson Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

jihoonson commented Aug 1, 2024 • edited Loading

jihoonson commented Aug 1, 2024 • edited Loading

jlowe left a comment

Choose a reason for hiding this comment

jihoonson commented Aug 1, 2024 • edited Loading

jihoonson commented Aug 1, 2024

jihoonson commented Aug 2, 2024

jlowe commented Aug 2, 2024

gerashegalov commented Aug 2, 2024 • edited Loading

jihoonson commented Aug 8, 2024

jihoonson commented Aug 8, 2024

gerashegalov commented Aug 8, 2024

gerashegalov commented Aug 8, 2024

jihoonson commented Aug 9, 2024

gerashegalov left a comment

Choose a reason for hiding this comment

gerashegalov Aug 9, 2024

Choose a reason for hiding this comment

jihoonson Aug 9, 2024

Choose a reason for hiding this comment

jihoonson commented Aug 9, 2024

jihoonson commented Aug 9, 2024

jihoonson commented Aug 9, 2024

jihoonson commented Aug 13, 2024

jihoonson commented Aug 14, 2024

jihoonson commented Aug 15, 2024

jihoonson Aug 1, 2024 •

edited

Loading

jihoonson commented Aug 1, 2024 •

edited

Loading

jihoonson commented Aug 1, 2024 •

edited

Loading

jihoonson commented Aug 1, 2024 •

edited

Loading

gerashegalov commented Aug 2, 2024 •

edited

Loading