Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove shims module [databricks] #4629

Merged
merged 17 commits into from
Jan 31, 2022
Merged

Conversation

razajafri
Copy link
Collaborator

@razajafri razajafri commented Jan 26, 2022

This is the first commit in a series of commit. The goal is to get rid of v1 shims usage and replace it with v2 usage

  • Source files from shims module were moved to sql-plugin module
  • Test files were moved to test folder inside sql-plugin module
  • Added the ability to select test files based on profile for testing shims
  • Moved README.md from shims module to docs/dev/ and renamed it to shims.md.
  • Modified the relative links in the doc to match the new location

Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@sameerz sameerz added the task Work required that improves the product but is not user facing label Jan 26, 2022
@sameerz sameerz added this to the Jan 10 - Jan 28 milestone Jan 26, 2022
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri razajafri marked this pull request as ready for review January 27, 2022 00:37
@razajafri
Copy link
Collaborator Author

build

Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

docs/dev/shims.md Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri razajafri changed the title Remove shims module Remove shims module [databricks] Jan 27, 2022
@razajafri
Copy link
Collaborator Author

build

Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri razajafri changed the base branch from branch-22.04 to branch-22.02 January 27, 2022 22:56
@razajafri razajafri changed the base branch from branch-22.02 to branch-22.04 January 27, 2022 22:56
@razajafri
Copy link
Collaborator Author

build

jlowe
jlowe previously approved these changes Jan 27, 2022
@razajafri
Copy link
Collaborator Author

CI failed due to [ERROR] Failed to execute goal on project rapids-4-spark-udf_2.12: Could not resolve dependencies for project com.nvidia:rapids-4-spark-udf_2.12:jar:22.04.0-SNAPSHOT: Could not find artifact com.nvidia:rapids-4-spark-shims-spark312db_2.12:jar:22.04.0-SNAPSHOT in snapshots-repo (https://oss.sonatype.org/content/repositories/snapshots) -> [Help 1]

Are we missing some jars for 22.04 for db?

@pxLi
Copy link
Collaborator

pxLi commented Jan 28, 2022

CI failed due to [ERROR] Failed to execute goal on project rapids-4-spark-udf_2.12: Could not resolve dependencies for project com.nvidia:rapids-4-spark-udf_2.12:jar:22.04.0-SNAPSHOT: Could not find artifact com.nvidia:rapids-4-spark-shims-spark312db_2.12:jar:22.04.0-SNAPSHOT in snapshots-repo (https://oss.sonatype.org/content/repositories/snapshots) -> [Help 1]

Are we missing some jars for 22.04 for db?

nope, the rapids-4-spark-shims-spark301db&rapids-4-spark-shims-spark312db_2.12 was supposed be built with in this CI (but this PR remove the 301db and 312db shims), and we never deploy nightly shims to sonatype snapshot repo.

failed at Building RAPIDS Accelerator for Apache Spark Scala UDF Plugin 22.04.0-SNAPSHOT [4/9]. This may require some dependency update. And there are still a few other modules have dep of
rapids-4-spark-shims-${spark.version.classifier}_${scala.binary.version}

FYI for pre-merge, since we have old nightly snapshot of shims on URM, so test in blossom could pass the build (which is not correct actually). And on DB runtime, it correctly failed as no those deps available.

@jlowe
Copy link
Member

jlowe commented Jan 28, 2022

Ah, right, I totally missed in the review that a number of other poms (aggregator, tests, etc.) need to be updated, as they explicitly depend on the shims artifact being removed.

@razajafri you can build in a clean repo, either by cleaning out your local ~/.m2 or using something like -Dmaven.repo.local=/tmp/m2temprepo to reproduce these types of errors locally.

Signed-off-by: Raza Jafri <rjafri@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

razajafri commented Jan 28, 2022

Ah, right, I totally missed in the review that a number of other poms (aggregator, tests, etc.) need to be updated, as they explicitly depend on the shims artifact being removed.

@razajafri you can build in a clean repo, either by cleaning out your local ~/.m2 or using something like -Dmaven.repo.local=/tmp/m2temprepo to reproduce these types of errors locally.

aah! of course I should've removed the dependency from other modules but I don't get how the CI was passing until I built on databricks. Does the CI reuse previously used maven repo? Nevermind, read @pxLi comment above. This is something we should probably fix

Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

sql-plugin/pom.xml Outdated Show resolved Hide resolved
sql-plugin/pom.xml Outdated Show resolved Hide resolved
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

@tgravescs
Copy link
Collaborator

build

@razajafri razajafri merged commit b7089c1 into NVIDIA:branch-22.04 Jan 31, 2022
@razajafri razajafri deleted the shim-work-2 branch January 31, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task Work required that improves the product but is not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants