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

Add jar argument to spark_rapids CLI #902

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

amahussein
Copy link
Collaborator

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

Fixes #901

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

Fixes NVIDIA#901
@amahussein amahussein added feature request New feature or request user_tools Scope the wrapper module running CSP, QualX, and reports (python) labels Apr 4, 2024
@amahussein amahussein self-assigned this Apr 4, 2024
Copy link
Collaborator

@nartal1 nartal1 left a comment

Choose a reason for hiding this comment

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

Thanks @amahussein ! LGTM.
Have a question:
What should be the behavior if the tools_jar version doesn't match with user-tools. i.e if the user accidently provides the old tools_jar version path, should we throw an error OR let it go through using the old jar itself? Or do we already have a check in place for this.

@amahussein
Copy link
Collaborator Author

Thanks @amahussein ! LGTM. Have a question: What should be the behavior if the tools_jar version doesn't match with user-tools. i.e if the user accidently provides the old tools_jar version path, should we throw an error OR let it go through using the old jar itself? Or do we already have a check in place for this.

Since, we cannot verify the version from the file's name, the way to do it is to process the POM file inside the jar to check the version. This implies that users with custom builds/versioning won't be able to use their jars against the CLI. Rendering the feature to be useless.

So, I would say that it is user's responsibility. Even if we process the pom to display a warning without completely bailing out, the benefit is not worth the effort IMHO.

Copy link
Collaborator

@cindyyuanjiang cindyyuanjiang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @amahussein!

Copy link
Collaborator

@parthosa parthosa left a comment

Choose a reason for hiding this comment

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

Thanks @amahussein.

@amahussein amahussein merged commit e8bdb33 into NVIDIA:dev Apr 4, 2024
16 checks passed
@amahussein amahussein deleted the spark-rapids-tools-901 branch April 4, 2024 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request user_tools Scope the wrapper module running CSP, QualX, and reports (python)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Add jar argument to spark_rapids CLI
4 participants