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.

Sign up for GitHub

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

Jump to bottom

Use dist/pom file as source of truth for spark versions #6437

Merged
merged 30 commits into from
Sep 21, 2022
Merged

Use dist/pom file as source of truth for spark versions #6437

merged 30 commits into from
Sep 21, 2022

Conversation

YanxuanLiu
Copy link
Collaborator

@YanxuanLiu YanxuanLiu commented Aug 29, 2022 •
edited
Loading

fixes #5086

Signed-off-by: YanxuanLiu [email protected]

snapshot and released spark shims versions list should take dist/pom file as source of truth instead of define in multiple places with hard code.

modifications:

  1. removed hard code spark shims versions
  2. run maven command to get spark shims versions from pom file
  3. optimized repeatable test process with loop and conditions for different spark shim versions

@YanxuanLiu
get spark shim version from pom
bb5d736 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
correct comments
dfc4c0d 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu YanxuanLiu requested a review from pxLi August 29, 2022 04:18
@pxLi
Copy link
Collaborator

pxLi commented Aug 30, 2022

build

@sameerz
Copy link
Collaborator

sameerz commented Aug 30, 2022

Please update the PR description to describe what is changing. The title will go into the changelog, so a good description is important.

pxLi
pxLi reviewed Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@pxLi pxLi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

as Sameer said. Please change the title to a meaningful one and update desc, thx~

jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/version-def.sh Outdated Show resolved Hide resolved
@YanxuanLiu YanxuanLiu changed the title Fix issue 5086 Use dist/pom file as source of truth for spark versions Sep 5, 2022
@YanxuanLiu YanxuanLiu marked this pull request as ready for review September 5, 2022 07:21
pxLi
pxLi reviewed Sep 5, 2022
View reviewed changes
jenkins/version-def.sh Outdated Show resolved Hide resolved
@YanxuanLiu
Copy link
Collaborator Author

build

@sameerz sameerz added the build Related to CI / CD or cleanly building label Sep 5, 2022
tgravescs
tgravescs reviewed Sep 6, 2022
View reviewed changes
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/version-def.sh Show resolved Hide resolved
gerashegalov
gerashegalov requested changes Sep 7, 2022
View reviewed changes
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/version-def.sh Outdated Show resolved Hide resolved
jenkins/version-def.sh Outdated Show resolved Hide resolved
@YanxuanLiu
fix some its
71bdb82 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
add func to get versions from pom
9c236aa 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
Copy link
Collaborator Author

build

gerashegalov
gerashegalov reviewed Sep 7, 2022
View reviewed changes
Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

Looks good. The only real comment is to try to get rid of "31*". Other comments are nits.

jenkins/common.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
@YanxuanLiu
add echo in yml for testing
1d940d1 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
fix bug
0fb874b 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
add SCRIPT_PATH for wrapper call
6d96fdd 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
add comment
ce25240 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
add comment
aa8a015 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
Copy link
Collaborator Author

build

gerashegalov
gerashegalov previously approved these changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

LGTM

.github/workflows/mvn-verify-check.yml Show resolved Hide resolved
@pxLi
Copy link
Collaborator

pxLi commented Sep 14, 2022

build

@pxLi
Copy link
Collaborator

pxLi commented Sep 14, 2022

retrigger after the regex fix

pxLi
pxLi reviewed Sep 14, 2022
View reviewed changes
jenkins/common.sh Outdated
@@ -0,0 +1,23 @@
#!/bin/bash
#
# Copyright (c) 2020-2022, NVIDIA CORPORATION. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

license: # Copyright (c) 2022

pxLi
pxLi reviewed Sep 14, 2022
View reviewed changes
Copy link
Collaborator

@pxLi pxLi left a comment

Choose a reason for hiding this comment

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

LGTM. Would be nice to wait for the premerge run and check the actual log

pxLi
pxLi reviewed Sep 14, 2022
View reviewed changes
jenkins/spark-premerge-build.sh Outdated
# build and run unit tests on one 3.1.X version (base version covers this), one 3.2.X and one 3.3.X version
if [[ "${TEST_SPARK_SHIM_VERSIONS[*]}" =~ "$version" ]]; then
# build and run unit test
if [[ "${SPARK_SHIM_VERSIONS_TEST[@]}" =~ "$version" ]]; then
Copy link
Collaborator

@pxLi pxLi Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

another thing to mention is that we separate some UT to ci_2 stage for balancing the duration of different stages, I think we could still have some hardcode exception here to put a heads-up of versions moved to other paralleled stages
https://github.com/NVIDIA/spark-rapids/blob/branch-22.10/jenkins/spark-premerge-build.sh#L136-L139

would be better to have premergeUT1 and premergeUT2

@YanxuanLiu
Copy link
Collaborator Author

build

@YanxuanLiu
skip base version
1d265ee 
Signed-off-by: YanxuanLiu <[email protected]>
@YanxuanLiu
add comment
2687f44 
Signed-off-by: YanxuanLiu <[email protected]>
@pxLi
Copy link
Collaborator

pxLi commented Sep 14, 2022

build

@pxLi
Copy link
Collaborator

pxLi commented Sep 14, 2022

failed unrelated #6558. will retrigger after the issue is resolved

pxLi
pxLi previously approved these changes Sep 14, 2022
View reviewed changes
gerashegalov
gerashegalov reviewed Sep 14, 2022
View reviewed changes
jenkins/spark-premerge-build.sh Outdated
$MVN_INSTALL_CMD -Dbuildver=$version
fi
done

# enable UTF-8 for regular expression tests
env -u SPARK_HOME LC_ALL="en_US.UTF-8" $MVN_CMD $MVN_URM_MIRROR -Dbuildver=320 test $MVN_BUILD_ARGS \
Copy link
Collaborator

@gerashegalov gerashegalov Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

another hardcoding overlooked: Should it be $SPARK_SHIM_VERSIONS_PREMERGE_UT_1 or some dedicated variable. Would be good if didn't have to come back to this file when we are going to update versions in dist/pom.

@gerashegalov
Copy link
Collaborator

build

@pxLi
Copy link
Collaborator

pxLi commented Sep 21, 2022

build

@pxLi
Copy link
Collaborator

pxLi commented Sep 21, 2022

Looks like previous trigger met internal gitlab 500 error and fail to start the corresponding jenkins pipeline

@pxLi pxLi merged commit 6d3f66c into NVIDIA:branch-22.10 Sep 21, 2022
@pxLi pxLi mentioned this pull request Sep 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgravescs tgravescs tgravescs left review comments

@pxLi pxLi pxLi approved these changes

@gerashegalov gerashegalov gerashegalov approved these changes

@jlowe jlowe Awaiting requested review from jlowe jlowe is a code owner

@revans2 revans2 Awaiting requested review from revans2 revans2 is a code owner

@GaryShen2008 GaryShen2008 Awaiting requested review from GaryShen2008 GaryShen2008 is a code owner

@NvTimLiu NvTimLiu Awaiting requested review from NvTimLiu NvTimLiu is a code owner

@zhanga5 zhanga5 Awaiting requested review from zhanga5

Assignees
No one assigned
Labels
build Related to CI / CD or cleanly building
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] use dist/pom files as source of truth for spark versions
5 participants
@YanxuanLiu @pxLi @sameerz @gerashegalov @tgravescs