Initial implementation for WorkflowUpdater component #11795
Conversation
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
I think this PR is good for a review. I am still working on the unit test, but I don't plan to make any other major changes in this pull request. @todor-ivanov @vkuznet please have a look whenever you can. |
| """ | ||
| # call the base class | ||
| super(Harness, self).__init__() | ||
| print("WorkflowUpdater.__init__") |
There was a problem hiding this comment.
why do you need print here and why not to use logger instead?
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
| thisPU = {"pileupName": puItem['pileupName'], | ||
| "customName": puItem['customName'], | ||
| "rses": puItem['currentRSEs'], | ||
| "blocks": []} |
There was a problem hiding this comment.
I see that blocks all the time will be an empty list, why do you need to construct it here?
There was a problem hiding this comment.
My goal is to populate this variable with a list of block names in a future pull request. Just so we have a data structure with all the relevant information for a given pileup dataset.
There was a problem hiding this comment.
In this case please change docstring accordingly to avoid confusion. For instance, I suggest to make concrete estimates using approximate time spent in rucio, e.g. O(1-10sec), multiplied by total number of given workflows, i.e. how much time we'll spend for 100 workflows (which is what I expect as average).
There was a problem hiding this comment.
I updated the docstring. I can also add a comment of expected time to be spent on that, but as things change, I am sure it will simply become outdated and not meaningful.
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
| still unfinished, then it is considered an unfinished/active workflow. | ||
| """ | ||
|
|
||
| sql = """SELECT wmbs_workflow.name, wmbs_workflow.spec |
There was a problem hiding this comment.
I know that it may be late in a WMCore game, but all SQL statement should not be hard-coded in the code, it is much better to keep them in templates such that we may easily change them without touch the code (for the record this is done in dbs2go codebase). This also allows to profile SQL easily and benchmark it without having a code.
There was a problem hiding this comment.
I might have already seen a GH reporting it, if not, do you feel like opening a dedicated issue for tackling this? I'd rather have a dedicated issue such that, whenever we change how DAOs are defined, that we make it consistency across the code (and potentially involving T0 as well).
There was a problem hiding this comment.
Yes, it can go to separate issue, and the way to make it backward compatible is the following. The DAO APIs should take optional file parameter and read from a file a template. Then, the refactoring can be done to move SQL statements into templates. I'm fine to have it as a separate issue though.
| Oracle implementation of Workflow.GetUnfinishedWorkflows | ||
| """ | ||
|
|
||
| from WMCore.WMBS.MySQL.Workflow.GetUnfinishedWorkflows import GetUnfinishedWorkflows as MySQLGetUnfinishedWorkflows |
There was a problem hiding this comment.
probably you need to break line to make it more readable and pylint/pep complaint to 80 characters.
| workflow=mergeWorkflow, | ||
| split_algo="ParentlessMergeBySize") | ||
|
|
||
| file1 = File(lfn="file1", size=1024, events=1024, first_event=0, locations={"T2_CH_CERN"}) |
There was a problem hiding this comment.
since locations dict does not change why not to define it as local variable and then use it everywhere?
There was a problem hiding this comment.
Let me change it real quick.
|
Jenkins results:
|
|
Jenkins results:
|
|
@vkuznet Valentin, I have not yet squashed those commits. Please review the code again and then I can squash it. Or let me know if you want me to squash it before your review. Thanks |
|
Jenkins results:
|
fix default config and polling cycle Parse new MSPILEUP_URL component configuration
| puWflows = self.findWflowsWithPileup(wflowSpecs) | ||
|
|
||
| # otherwise, move on retrieving pileups | ||
| pileupMap = self.getPileupDocs() |
There was a problem hiding this comment.
If pileupMap is not used in return there is no need to make this local variable, you should either change it to _ or not assign anything at all from right hand side of this expression.
There was a problem hiding this comment.
As we work in the other tickets relevant to this component, it will be used.
| thisPU = {"pileupName": puItem['pileupName'], | ||
| "customName": puItem['customName'], | ||
| "rses": puItem['currentRSEs'], | ||
| "blocks": []} |
There was a problem hiding this comment.
In this case please change docstring accordingly to avoid confusion. For instance, I suggest to make concrete estimates using approximate time spent in rucio, e.g. O(1-10sec), multiplied by total number of given workflows, i.e. how much time we'll spend for 100 workflows (which is what I expect as average).
| :param listSpecs: a list of dictionary with workflow name and spec path | ||
| :return: a list with the workflow names that require pileup | ||
| """ | ||
| wflowsWithPU = [] |
There was a problem hiding this comment.
Based on your numbers the list will be 8.8KB and would be copied from place to place. And, you can achieve len(gen) simply adding another local counter variable. As I said I'm not against the list but I have no idea how it will be used afterwards and I'm trying to estimate the code of having large list go through multiple function calls.
| def findRucioBlocks(self, uniquePUList, pileupMap): | ||
| """ | ||
| Given a list of unique pileup dataset names, list all of | ||
| their blocks in Rucio |
There was a problem hiding this comment.
I suggest to add appropriate comment into doc string about potential slowness of this function especially with increasing number of processed workflows. Moreover, I suggest to add @timeFunction decorator to explicitly dump time spent in this function such that we we'll not be guessing but rather can look it up from the log after operations.
| self.testInit.generateWorkDir(config) | ||
|
|
||
| # First the general stuff | ||
| config.section_("General") |
There was a problem hiding this comment.
I want to point out as a general comment here. Usage of custom WMCore configuration significantly impact code portability to other frameworks, like Flask, etc. There is no need to use it everywhere, and more standard format for configuration, like, .ini, .json, .yaml is a better choice. Having relying on custom format add extra layer of dependency without much of gain in code structure or functionality, and make it impossible to port (parts of) code to other languages which does not have WMCore python based configuration. Said that, there is nothing here to fix or address, and my comment is a general observation.
|
Jenkins results:
|
|
@vkuznet I addressed your relevant concerns in the last commit (to be eventually squashed). |
|
Jenkins results:
|
|
Thank you for the prompt reviews, Valentin. Yes, the idea is to get it merged and resume activities on this component and other related tickets. I am going to squash the commits accordingly; and also get some pylint fixed/refactored to make it look better. |
New DAOs for finding active workflows Return spec path from the DAO Load spec file and find whether pileup is required or not fix logger object when instantiating Rucio Change Rucio account to wma_test Valentins suggestions, part 1 use GET method instead of POST Fix some data structures; time findRucioBlocks pylint fixes in WorkflowUpdater
|
Jenkins results:
|
unit tests - rename testWorkload function by createTestWorkload fix unit tests calls and imports use wma_test in unit tests Valentins unit test suggestions pylint fixes for test package
|
Jenkins results:
|
Fixes #11733
Status
Ready
Description
This pull request provides the foundation of a new component called
WorkflowUpdater. Further details are:In addition, there are deployment changes and it also requires changes to the secrets file with new urls.
Extra, tell git to ignore
.vscodedirectory/files in the repository (for VSCode users).NOTE that this component is expected to be functional, but still missing its real functionality as other tickets still need to be addressed.
Is it backward compatible (if not, which system it affects?)
NO, new component!
Related PRs
None
External dependencies / deployment changes
It has 3 dependencies so far:
MSPILEUP_URLcustomName, provided in: Add support for customName (custom container DID) #11765