Provide concurrent implementation for wmstatsserver jobdetail API #11885
Conversation
amaltaro
added
PR: Do not merge yet
PR: Work in progress
PR: NEVER MERGE
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
amaltaro
changed the title
amaltaro
removed
PR: Do not merge yet
PR: Work in progress
PR: NEVER MERGE
| startKey = row["key"][:4] | ||
| endKey = [] | ||
| site = row["key"][4] | ||
| if site: |
There was a problem hiding this comment.
Would it make sense to use try-except in place of the if statement?
There was a problem hiding this comment.
I think it can be either None or an empty string. In such cases, we do not want to have it as part of the query, hence we don't append it.
| callRuntime = round((time.time() - reqStart), 3) | ||
| msg = f"Retrieved job details (pycurl mode: {usePycurl}) for {requestName} in " | ||
| msg += f"{callRuntime} seconds, with a total of {len(results['rows']) + 1} CouchDB calls" | ||
| print(msg) |
There was a problem hiding this comment.
Note that I put this print statement here because it's the only way to get this logged into the service access_log. If we prefer, I can remove it. But I think it's beneficial to keep it around for a while, at least.
|
@vkuznet Valentin, I finally managed to get back to this and applied some of your suggestions. The files look ugly somehow with those very large queries and/or output, but if it makes things more clear, let's keep it. |
|
Jenkins results:
|
| :return: dictionary with job detail information | ||
|
|
||
| A decoded example of this query would be: | ||
| scurl "https://cmsweb-test9.cern.ch/couchdb/wmstats/_design/WMStatsErl3/_view/jobsByStatusWorkflow?reduce=false&include_docs=true&startkey=["pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876","/pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876/DataProcessing","jobfailed",8006,"T2_US_Wisconsin"]&endkey=["pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876","/pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876/DataProcessing","jobfailed",8006,"T2_US_Wisconsin",{}]&limit=1&stale=update_after" |
There was a problem hiding this comment.
I suggest to replace actual workflow names by shorter examples, e.g.
["workflow_name","/workflow_name/DataProcessing","jobfailed",8006,"T2_US_XXX"]&endkey=["workfow_name","/workflow_name/DataProcessing","jobfailed",8006,"T2_US_XXX",{}]&limit=1&stale=update_after"
and then add how reader should treat workflow_name as placeholder of actual value.
There was a problem hiding this comment.
Ok, it will be shorter that way. I am pushing my code in soon.
| A decoded example of this query would be: | ||
| scurl "https://cmsweb-test9.cern.ch/couchdb/wmstats/_design/WMStatsErl3/_view/jobsByStatusWorkflow?reduce=false&include_docs=true&startkey=["pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876","/pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876/DataProcessing","jobfailed",8006,"T2_US_Wisconsin"]&endkey=["pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876","/pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876/DataProcessing","jobfailed",8006,"T2_US_Wisconsin",{}]&limit=1&stale=update_after" | ||
| while encoding the url would result in: | ||
| scurl "https://****.cern.ch/couchdb/wmstats/_design/WMStatsErl3/_view/jobsByStatusWorkflow?reduce=false&include_docs=true&startkey=%5B%22pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876%22%2C+%22%2Fpdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876%2FDataProcessing%22%2C+%22jobfailed%22%2C+8006%2C+%22T2_US_Wisconsin%22%5D&endkey=%5B%22pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876%22%2C+%22%2Fpdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876%2FDataProcessing%22%2C+%22jobfailed%22%2C+8006%2C+%22T2_US_Wisconsin%22%2C+%7B%7D%5D&limit=1&stale=update_after" |
There was a problem hiding this comment.
if you'll adopt my suggestion above, this one will change too
|
|
||
| Example of output is: | ||
| {"total_rows":4764135,"offset":4056263,"rows":[ | ||
| {"id":"12703ce5-b288-42cb-8658-f0b4a921406d-34","key":["pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876","/pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876/DataProcessing","jobfailed",8006,"T2_US_Wisconsin","https://cmsweb.cern.ch/couchdb/acdcserver","vocms0255.cern.ch",["Fatal Exception"]], |
There was a problem hiding this comment.
no need in actual ids, and you can easily change them to 123-xyz to shorter the output and make it better for readability, or even put uuid in the value of id.
| Example of output is: | ||
| {"total_rows":4764135,"offset":4056263,"rows":[ | ||
| {"id":"12703ce5-b288-42cb-8658-f0b4a921406d-34","key":["pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876","/pdmvserv_Run2017G_DoubleMuon_UL2017_MiniAODv2_BParking_230917_124108_9876/DataProcessing","jobfailed",8006,"T2_US_Wisconsin","https://cmsweb.cern.ch/couchdb/acdcserver","vocms0255.cern.ch",["Fatal Exception"]], | ||
| "value":{"id":"12703ce5-b288-42cb-8658-f0b4a921406d-34","rev":"4-90ba3bcfff9d8eb4dd16f023f8f45742"}, |
There was a problem hiding this comment.
no need for full revision id which can be sortened to something like 4-xyz.
|
@vkuznet my latest commit should cover your previous review, where I basically massaged the query and output examples. |
|
Jenkins results:
|
There was a problem hiding this comment.
My main concern is memory impact of jobDetailByTasksPycurl due to returned nested dictionary of results. Instead, I rather would like to see what is expected in upstream code as a data-structure and once it will be clear we may convert the code into a generator.
| :param serviceOpts: dictionary with CouchDB options (url, db name and couchapp name) | ||
| :return: dictionary with job detail information | ||
|
|
||
| A decoded example of this query would be: |
There was a problem hiding this comment.
Please either use placeholder or add explicit comment about jobfailed and 8006 values in queries shown below. Reader of the code may have wrong impression, e.g. look only at jobfailed, when in fact it may have different values. I would use JOB_STATUS, and EXIT_CODE as placeholders and then provide some values in examples below.
There was a problem hiding this comment.
Okay, to me it looks too much verbose to be added to the docstring, but here it is.
There was a problem hiding this comment.
thanks for update, it will pay off when someone will start refactoring the code. And, it show how complex the data structure is and how to address this complexity when we'll need refactoring.
| :param queries: list of CouchDB query parameters | ||
| :param limit: number of documents to retrieve from CouchDB | ||
| :param serviceOpts: dictionary with CouchDB options (url, db name and couchapp name) | ||
| :return: dictionary with job detail information |
There was a problem hiding this comment.
please provide structure of returned dictionary as it is very nested and complex.
There was a problem hiding this comment.
My second comment is that I would like to know if combined jobInfoDoc is really required here and would it be beneficial to convert this function to a generator to reduce memory footprint. But yielding a generator will affect other code re-factoring.
There was a problem hiding this comment.
Example structured is provided in this very same docstring.
On what concerns memory footprint, I am afraid yes, we do need to load everything in memory because of further data reorganization. In addition, a dictionary is returned, making it harder to send to the user piece by piece of information.
There was a problem hiding this comment.
Alan, I put this comment in exact place where clarification is required. I'm talking about structure of return jobInfoDoc dictionary and not structure of couchdb calls which you provided. As far as I can read what jobInfoDoc looks like it has the following structure:
{ workflow_name : {
task: {
job_status: {
exit_code: {
site: {.....}
}
}
}
}
}
In my opinion it would be useful to have it in docstring with full structure (including parts of site dictionary).
| thisUrl = f"{baseUrl}?{urlencode(options, doseq=True)}" | ||
| urlsPool.append(thisUrl) | ||
|
|
||
| jobInfoDoc = {} |
There was a problem hiding this comment.
instead of making one, very nested, dictionary, would it be better only to yield individual data-structures and convert this function to a generator?
There was a problem hiding this comment.
We don't have any benefits with that because data is further manipulated before it is sent back to the end user. In addition to what is done here, there is also this extra method manipulating/modifying it: https://github.com/dmwm/WMCore/pull/11885/files#diff-2f9af48630184a7dfd398437ef3c7ec5026da5d3a3c3813e39b5f5650421b69fR390
| jobDetails = nestedDictUpdate(jobDetails, jobInfo) | ||
| else: | ||
| # then it is sequential | ||
| for row in results['rows']: |
There was a problem hiding this comment.
If we'll consider converting jobDetailByTasksPycurl to a generator then code over here should be re-factored as well to be a generator. To make such transition easier the code below first should be put into individual function and then we can convert this function to a generator with common data-structure.
|
@vkuznet Valentin, I just wanted to point out that I have no intention of refactoring the data structure with this PR. Change of data structure here would impact the data consumed by end user clients (in addition to WM services). The only goal with this PR is to provide a concurrent implementation for the WMStatsReader |
|
Alan, if we will not refactor the code to generator kind, then the only issue I had is listed here: #11885 (comment), once updated I'm ok with changes. |
|
Valentin, I fixed your remaining question/request on this review. There is another comment that I think we can mark as resolved, but I will leave that around for now. On what concerns the generator and decrease in memory footprint, I feel like this will have to be tackled in a future PR focused on the actual data structure of WMStats server. Such that we make breaking changes (not backwards compatible) only once. |
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
| :param queries: list of CouchDB query parameters | ||
| :param limit: number of documents to retrieve from CouchDB | ||
| :param serviceOpts: dictionary with CouchDB options (url, db name and couchapp name) | ||
| :return: dictionary with job detail information |
There was a problem hiding this comment.
Alan, I put this comment in exact place where clarification is required. I'm talking about structure of return jobInfoDoc dictionary and not structure of couchdb calls which you provided. As far as I can read what jobInfoDoc looks like it has the following structure:
{ workflow_name : {
task: {
job_status: {
exit_code: {
site: {.....}
}
}
}
}
}
In my opinion it would be useful to have it in docstring with full structure (including parts of site dictionary).
|
Jenkins results:
|
|
@vkuznet Valentin, I updated the docstring with your request. I also noticed that a nestedUpdate call is not needed when using the pycurl implementation, so I removed it in the (now) 2nd commit. If everything looks Okay to you, I will squash commits once again and get these changes in. |
| :param serviceOpts: dictionary with CouchDB options (url, db name and couchapp name) | ||
| :return: dictionary with job detail information | ||
|
|
||
| A decoded example of this query would be: |
There was a problem hiding this comment.
thanks for update, it will pay off when someone will start refactoring the code. And, it show how complex the data structure is and how to address this complexity when we'll need refactoring.
replace logging by print function refactor print statements Single print message with workflow name, time and num of calls Created methods to make concurrent calls with pycurl fixed key names fixed the baseUrl properly parse the response and keys Correctly set errorCount Need a sub-set of the keys for comparison Remove some debugging Handle HTTP errors Separate methods in their own module: WMStatsPycurl Apply Valentins suggestions more Valentins suggestions for shortening the line length more Valentins requests provide example of jobInfoDoc No need for nestedDictUpdate when using pycurl
|
Jenkins results:
|
|
commits squashed and it's good to go. Thanks for the reviews, Valentin. |
Fixes #11571
Status
ready
Description
This PR started as a characterization process of the
jobdetailWMStats REST API, which ended up with a sequential mode for retrieving data from CouchDB.This PR provides an alternative implementation - enabled by default - which results in concurrent calls to CouchDB, thus substantially decreasing the total time to retrieve data from CouchDB/wmstats.
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None