Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[BEAM-6908] Refactor Python performance test groovy file for easy configuration #8518
[BEAM-6908] Refactor Python performance test groovy file for easy configuration #8518
Changes from 1 commit
bfb0bbe
8cde119
116ba25
f16c2fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of this? I am confused by how this is actually used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a benchmark flag defined in here. Basically it's the path of Gradle module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know how to set this correctly? It seems not intuitive...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can rename it something like
itGradleModule
if helps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but how do we know which gradle module to select? I see that you used a different value for Py2 and Py3 benchmark, how did you pick those specific ones? How does a person writing an new benchmark decides how to fill this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think people should know how Perfkit
beam_integration_benchmark
works before configuring in Jenkins. Probably we need better document for that, and also happy to sync with you offline for more details.For you question,
beam_integration_benchmark
uses Gradle taskintegrationTest
which can be enabled throughenablePythonPerformanceTest
. Sobeam_it_module
is the Gradle project whereintegrationTest
located.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per offline discussion, let's add a comment here:
Gradle project that defines 'runIntegrationTest' task. This task is executed by Perfkit Beam benchmark launcher.
This task can be added by enablePythonPerformanceTest() defined in BeamModulePlugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can always use the same location here. As I mentioned before, there is no difference which interpreter version to use to create tarball.
If we must specify location per suite, perhaps we could have one parameter, such as
testRoot
and evaluatesdkLocation
anditModule
value from that parameter. As mentioned above I also am not sure whatitModule
is for.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, tar file is generated in build directory of the Gradle module where IT is located. We need to specify location per test. We can populate sdkLocation from itModule directly and in the future we could refactor Gradle build to generate tar file only once. markflyhigh#6 is a draft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is my understanding right that each performance test configuration can run only one IT? We might want to always include 'wordcount' in configuration flags or always omit if the suite will later include other benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it can run multiple ITs if you want.
itClass
will be passed toclassname
of this function and eventually to-Dtests
of the Gradle invocation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but we will not get one test reading per each test, instead we will run both tests, and get a total runtime, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. This flag defines what tests run in a Gradle execution, and the benchmark will evaluate whole Gradle execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in this case it's one test per configuration, so we may want to call it "WordCount" benchmark, instead of generic 'Performance test'.
Also, where do we specify the input for the WC pipeline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg. done
This file was deleted.