Make '--platform' argument mandatory in qualification and profiling CLI to prevent incorrect behavior #1463
Conversation
Signed-off-by: Partho Sarthi <[email protected]>
parthosa
added
bug
There was a problem hiding this comment.
Thanks @parthosa! I saw a lot of test cases are converted from should pass to should fail because platform is missing. Shall we add back some should pass test cases with platform provided?
| # for qualification, cost savings should be enabled because cluster is provided | ||
| self.validate_tool_args(tool_name=tool_name, tool_args=tool_args, | ||
| cost_savings_enabled=True, | ||
| expected_platform=csp) |
There was a problem hiding this comment.
Can we add back this should pass test case when platform is available?
There was a problem hiding this comment.
Interesting observation @cindyyuanjiang !
I am fine with whichever you folks agree on.
There was a problem hiding this comment.
I think all of these tests already include a should_pass case with the platform defined in the previous block.
These updated tests were expected to pass when the platform is not defined. Now, these tests are expected to fail when the platform is not defined.
Hence, I dont think we need to add more tests for should_pass. Let me know your thoughts.
There was a problem hiding this comment.
@parthosa thanks, I revisited the files and confirmed the other test cases.
| # for qualification, cost savings should be enabled because cluster is provided | ||
| self.validate_tool_args(tool_name=tool_name, tool_args=tool_args, | ||
| cost_savings_enabled=True, | ||
| expected_platform=csp) |
There was a problem hiding this comment.
Interesting observation @cindyyuanjiang !
I am fine with whichever you folks agree on.
Fixes #1462.
This PR makes the
--platformargument mandatory to address issues with incorrect platform detection. Our current platform detection logic may inaccurately identify the platform, leading to incorrect behavior and speed up estimations. For more details, refer to the issue description.Note:
generate_instance_descriptionsorpredictionthat require a platform are not affected.Changes
Enforcing Platform Argument Requirement:
user_tools/src/spark_rapids_tools/cmdli/argprocessor.py: Added a new rejected case for the missing platform argument in thedefine_invalid_arg_casesmethod.Test Case Updates:
user_tools/tests/spark_rapids_tools_ut/test_tool_argprocessor.py: Updated thetest_with_platform_with_eventlogsmethod to ensure it fails when the platform argument is not provided.user_tools/tests/spark_rapids_tools_ut/test_tool_argprocessor.py: Updated thetest_with_platform_with_eventlogs_with_jar_filesmethod to ensure it fails when the platform argument is not provided.user_tools/tests/spark_rapids_tools_ut/test_tool_argprocessor.py: Updated thetest_with_platform_with_cluster_propsmethod to ensure it fails when the platform argument is not provided.user_tools/tests/spark_rapids_tools_ut/test_tool_argprocessor.py: Updated thetest_with_platform_with_cluster_props_with_eventlogsmethod to ensure it fails when the platform argument is not provided.user_tools/tests/spark_rapids_tools_ut/test_tool_argprocessor.py: Updated thetest_with_platform_with_autotuner_with_eventlogsmethod to ensure it fails when the platform argument is not provided.