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
Conversation
Run Seed Job |
Run Python Performance Test |
Run Python35 Performance Test |
+R: @tvalentyn @yifanzou |
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.
LGTM
Run Seed Job |
Run Python35 Performance Test |
PTAL @yifanzou |
String sdk = 'python' | ||
String bigqueryTable | ||
String itClass | ||
String itModule |
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 task integrationTest
which can be enabled through enablePythonPerformanceTest
. So beam_it_module
is the Gradle project where integrationTest
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.
bigqueryTable : 'beam_performance.wordcount_py35_pkb_results', | ||
skipPrebuild : true, | ||
pythonSdkLocation : 'test-suites/dataflow/py35/build/apache-beam.tar.gz', | ||
itClass : 'apache_beam.examples.wordcount_it_test:WordCountIT.test_wordcount_it', |
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 to classname
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
jobTriggerPhrase : 'Run Python35 Performance Test', | ||
bigqueryTable : 'beam_performance.wordcount_py35_pkb_results', | ||
skipPrebuild : true, | ||
pythonSdkLocation : 'test-suites/dataflow/py35/build/apache-beam.tar.gz', |
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 evaluate sdkLocation
and itModule
value from that parameter. As mentioned above I also am not sure what itModule
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.
Thanks, @markflyhigh ! |
1a38f42
to
798ca0a
Compare
Updated comments to parameters in |
Run Seed Job |
Run Python35 Performance Test |
798ca0a
to
116ba25
Compare
Run Seed Job |
Run Python35 Performance Test |
String sdk = 'python' | ||
String bigqueryTable | ||
String itClass | ||
String itModule |
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...
bigqueryTable : 'beam_performance.wordcount_py35_pkb_results', | ||
skipPrebuild : true, | ||
pythonSdkLocation : 'test-suites/dataflow/py35/build/apache-beam.tar.gz', | ||
itClass : 'apache_beam.examples.wordcount_it_test:WordCountIT.test_wordcount_it', |
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?
PTAL @tvalentyn |
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.
Thanks, a few more comments.
benchmarks : testConfig.benchmarkName, | ||
bigquery_table : testConfig.resultTable, | ||
beam_it_class : testConfig.itClass, | ||
beam_it_module : testConfig.itModule, | ||
beam_prebuilt : testConfig.prebuilt.toString(), | ||
beam_prebuilt : 'true', |
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.
Could we add a comment above why this value needs to be true?
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.
done
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.
The comment says // always true for Python tests
but does not explain why this needs to be true, so this configuration bit remains a little cryptic... Could we add a simple explanation?
bigqueryTable : 'beam_performance.wordcount_py35_pkb_results', | ||
skipPrebuild : true, | ||
pythonSdkLocation : 'test-suites/dataflow/py35/build/apache-beam.tar.gz', | ||
itClass : 'apache_beam.examples.wordcount_it_test:WordCountIT.test_wordcount_it', |
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?
String sdk = 'python' | ||
String bigqueryTable | ||
String itClass | ||
String itModule |
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?
44b4f44
to
cd4793a
Compare
PTAL @tvalentyn |
cd4793a
to
bd49953
Compare
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.
Thanks, @markflyhigh, overall LGTM.
Left one comment inline, also, is there a link on somewhere on Beam website that points to results produced by these benchmarks?
If not, could we add it please?
CC: @manisha252 who is working on performance test infrastructure and may have additional input here. |
Thank you @tvalentyn. To your question, results are stored in Bigquery table which should be accessible from UI or gcloud commandline. Different job is likely to use own table and configured in |
I meant that we should have some pointers in Beam documentation that can point to existence of these benchmarks and dashboards they produce. |
Run Seed Job |
Run Python35 WordCountIT Performance Test |
bd49953
to
f16c2fc
Compare
@tvalentyn I can add link of Bigquery table and dashboard in Beam doc. Also fixed the comment for Synced with @manisha252 offline and got approved for this change. |
…figuration (apache#8518) Refactor Python performance test groovy file for easy configuration.
Combine Python performance test groovy files into one with structured configuration block. Adding a new python performance test job can be simply as adding a
PerformanceTestConfigurations
intotestConfigurations
list.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.