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

Revert "Follow Up: Make '--platform' argument mandatory in CLI (#1473)" #1498

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Revert "Follow Up: Make '--platform' argument mandatory in CLI (#1473)" #1498

wants to merge 2 commits into from

Conversation

parthosa
Copy link
Collaborator

@parthosa parthosa commented Jan 10, 2025 •
edited
Loading

Fixes #1497.

In #1472, the --platform argument was made a required parameter in Fire. In our tests, it was found that the short flag -p for platform is no longer functioning.

Upon investigation, it seems that Fire does not support short flags for required arguments.

This PR reverts the above commit and makes the platform argument optional in Fire. This change will ensure compatibility with the -p flag.

The platform argument will still be validated internally to ensure its requirement is enforced.

Case 1 : Platform not provided

spark_rapids qualification  --eventlogs "/path/to/dataproc-cpu" --verbose

STDOUT


2025-01-10 13:35:16,011 INFO spark_rapids_tools.argparser: ...applying argument case: Missing Platform argument
2025-01-10 13:35:16,012 ERROR spark_rapids_tools.argparser: Validation err: 1 validation error for QualifyUserArgModel
  Cannot run tool cmd without platform argument. Re-run the command providing the platform argument.
  Error: [type=invalid_argument, input_value=ArgsKwargs((), {'eventlog...ols_config_path': None}), input_type=ArgsKwargs]

Case 2: Platform provided by --platform and -p

spark_rapids qualification --platform dataproc --eventlogs "/path/to/dataproc-cpu" --verbose
spark_rapids qualification -p dataproc --eventlogs "/path/to/dataproc-cpu" --verbose

STDOUT

Report Summary:
----------------------  -
Total applications      1
Processed applications  1
Top candidates          0
----------------------  -


Processing Completed!

@parthosa parthosa added bug Something isn't working user_tools Scope the wrapper module running CSP, QualX, and reports (python) labels Jan 10, 2025
@parthosa parthosa requested a review from amahussein January 10, 2025 22:14
@parthosa parthosa self-assigned this Jan 10, 2025
@parthosa
Fix license header
a7114bf 
Signed-off-by: Partho Sarthi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amahussein amahussein Awaiting requested review from amahussein

At least 1 approving review is required to merge this pull request.

Assignees

@parthosa parthosa

Labels
bug Something isn't working user_tools Scope the wrapper module running CSP, QualX, and reports (python)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Short flags for --platform does not work
1 participant
@parthosa