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

Fix core tool doc links and user qualification tool default argument values #931

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

cindyyuanjiang
Copy link
Collaborator

Fixes #929

Changes

  1. Fix stale links in core tool README
  2. Update default argument values for user qualification tool

@cindyyuanjiang cindyyuanjiang self-assigned this Apr 12, 2024
@cindyyuanjiang cindyyuanjiang added documentation Improvements or additions to documentation user_tools Scope the wrapper module running CSP, QualX, and reports (python) labels Apr 12, 2024
Copy link
Collaborator

@amahussein amahussein left a comment

Choose a reason for hiding this comment

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

Thanks @cindyyuanjiang
I am not sure we need to set the default value in every wrapper class. The code internally process the arguments because we might need to reset the default argument depending on the status of the execution.
That's why all the default values for estimation_model was set to None.
P.S: The spark_rapids_user_tools is going to be deprecated.

Comment on lines 43 to 44
filter_apps: str = QualFilterApp.tostring(QualFilterApp.get_default()),
estimation_model: str = QualEstimationModel.tostring(QualEstimationModel.get_default()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Handling the default is done in the ArgProcessor class because there is some logic that is required to calculate the default.
That's why it is set to None in the CLI to be able to distinguish between whether the user left the argument empty or did he actually set the value intentionally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @amahussein! I will update this.

@cindyyuanjiang
Copy link
Collaborator Author

Thanks @cindyyuanjiang I am not sure we need to set the default value in every wrapper class. The code internally process the arguments because we might need to reset the default argument depending on the status of the execution. That's why all the default values for estimation_model was set to None. P.S: The spark_rapids_user_tools is going to be deprecated.

Thanks @amahussein! I removed the default values for estimation_model. I kept the default value for filter_apps in spark_rapids to be consistent with the spark_rapids_user_tools, but we can change that if that's not the intended behavior.

The original thought for adding the default values is that when users run spark_rapids qualification -- --help they would see them.

Copy link
Collaborator

@amahussein amahussein left a comment

Choose a reason for hiding this comment

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

Thanks @cindyyuanjiang !
The filter_apps better stay None so that we handle the default value in one place .

@@ -40,7 +40,7 @@ def qualification(self,
platform: str = None,
target_platform: str = None,
output_folder: str = None,
filter_apps: str = None,
filter_apps: str = QualFilterApp.tostring(QualFilterApp.get_default()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue with filter_apps argument.
The filter_apps aregument has some complicated logic to get the default. It depends on the value of other arguments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @amahussein! Updated this.

Signed-off-by: cindyyuanjiang <[email protected]>
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 @cindyyuanjiang. LGTM

Copy link
Collaborator

@amahussein amahussein left a comment

Choose a reason for hiding this comment

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

Thanks @cindyyuanjiang
LGTME

@cindyyuanjiang cindyyuanjiang merged commit 5ee8606 into NVIDIA:dev Apr 12, 2024
15 checks passed
@cindyyuanjiang cindyyuanjiang deleted the spark-rapids-tools-929 branch April 12, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation 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.

Fix documentation stale links and discrepancies
3 participants