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

Spark rapids tools 1120 fea args #11

Merged
merged 7 commits into from
Jul 2, 2024

Conversation

bilalbari
Copy link
Collaborator

No description provided.

bilalbari added 4 commits July 2, 2024 10:05
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
@amahussein amahussein self-requested a review July 2, 2024 15:52
default = Some("."), descr = "Directory to write output to")

val outputFormat: ScallopOption[String] = opt[String](name = "outputFormat", short = 'o',
default = Some("tbl"), descr = "Format of output ( tbl, json)")
Copy link
Owner

Choose a reason for hiding this comment

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

  • More details about the options and what each one of them mean.
  • Suggest changing "tbl" to "text"
  • the short version 'o' should be for the outputDirectory. For this option, "'f'" would be more suitable
  • QQ why do we specify the name in all the options? shouldn't Scallop do that automatically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Added more description
  • Changed tbl to text. Current PR does not do any processing on output format. Later iteration will merge the JSON output changes
  • The short formats have been updated
  • The name option is just for verbosity. Saw online implementation using name param hence added it

Copy link
Owner

Choose a reason for hiding this comment

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

I belive if we leave it without name then scalaOpt will automatically convert the variable name.
For example, outputDirectory becomes output-directory
It will be better to use the snake-case because the tools main classes are following that standard. QualificationMain and ProfilingMain

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the name param to enable snake-case usage in params

warmUpIterations = warmUpIterations,
minNumIters = iterations)
benchmarker.addCase("QualificationBenchmark") { _ =>
mainInternal(new QualificationArgs(mainArgs),
Copy link
Owner

Choose a reason for hiding this comment

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

I am little bit confused here. If we initialize QualificationArgs on line 21, then what are those default arguments that are passed to the benchmarker variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clarified post discussion. Benchmarker arguments control iterations and warm up iterations. The passed ffunction to the benchmarker is what is benchmarked internally

Copy link
Owner

Choose a reason for hiding this comment

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

We will iterate on this later.

Signed-off-by: Sayed Bilal Bari <[email protected]>
Copy link
Collaborator Author

@bilalbari bilalbari left a comment

Choose a reason for hiding this comment

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

Review comments handled

warmUpIterations = warmUpIterations,
minNumIters = iterations)
benchmarker.addCase("QualificationBenchmark") { _ =>
mainInternal(new QualificationArgs(mainArgs),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clarified post discussion. Benchmarker arguments control iterations and warm up iterations. The passed ffunction to the benchmarker is what is benchmarked internally

warmUpIterations = warmUpIterations,
minNumIters = iterations)
benchmarker.addCase("QualificationBenchmark") { _ =>
mainInternal(new QualificationArgs(mainArgs),
Copy link
Owner

Choose a reason for hiding this comment

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

We will iterate on this later.

Signed-off-by: Sayed Bilal Bari <[email protected]>
Copy link
Collaborator Author

@bilalbari bilalbari left a comment

Choose a reason for hiding this comment

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

Updated with changes for review comments

@bilalbari bilalbari merged commit 8e3b18b into spark-rapids-tools-1120 Jul 2, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants