-
Notifications
You must be signed in to change notification settings - Fork 106
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
Continuous update of the pileup availability #11884
Conversation
Jenkins results:
|
Jenkins results:
|
fdac69e
to
080548b
Compare
Jenkins results:
|
080548b
to
a3caf41
Compare
Jenkins results:
|
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.
Valentin, I have not yet reviewed the actual logic of pileup json update. However, I left some reviews for your consideration.
Please also update the PR description with your own words of what is provided in this PR. There is no need for a very detailed description, but things like "Util method to find pileup json file in workflow sandbox; Fetch pileup data location from MSPileup; etc etc".
@@ -101,8 +289,49 @@ def algorithm(self, parameters=None): | |||
with CodeTimer("Rucio block resolution", logger=logging): | |||
self.findRucioBlocks(uniqueActivePU, msPileupList) | |||
|
|||
# TODO in future tickets: find the json files in the spec/sandbox area and tweak them | |||
# define where to look for sandbox tar balls | |||
idir = getattr(self.config.WorkflowUpdater, "componentDir") |
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.
Please move this to the __init__
method.
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.
why? The point is that I don't know what happen at run-time and when this area is created. In other words, does it part of WMA installation or is it part of run-time, i.e. when first job is processed. I think it belongs in a right place since I line below I check if it exists, and use '/data/srv/wmagent/current/install/wmagentpy3'
as fall back area. And if neither exist the algorithm cycle fails with proper log error.
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.
if this attribute does not exist, the component will actually fail to even start. This is not a runtime attribute, it is a deployment configuration/attribute. In other words, it is defined only once in an agent lifetime.
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.
Alan, it is not about component but rather about directory. Does directory exist before run time?
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, it does. I am only saying that whenever you read the component configuration, the ideal place for that is in the init method (or setup, when it exists).
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
Jenkins results:
|
Jenkins results:
|
Update the PR description, please. |
PR description is updated. |
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.
Valentin, in addition to the comments made along the code, please do use the report provided by Jenkins, as you will see that it points to errors in the current code.
I have to say that I am not very fond of using temporary directory for expanding the current sandbox, making the necessary updates, and compressing things again. I feel like it can one day give us a hard time. The only way that I can think of to replace that would be to completely refactor how pileup is transferred and accessed inside the job...
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
Jenkins results:
|
Jenkins results:
|
Alan, original PR was checked against Jenkins, only when you asked to re-arrange stuff I didn't check them since you said it was not complete review. Said that, jenkins now are passed where I filled out missing imports due to code re-arrangements. In your review you asked for few optimization which I find premature. I tried to address all your comments to state my view on this subject. |
Jenkins results:
|
Valentin, I think we covered these details last time we had a chat, but let me write a not complete and detailed logic here:
Some of this is already implemented in master, others are already implemented in this PR. But I believe there are changes still to be done in here. Regarding how exactly pileupconf.json needs to be updated, I think we discussed that and your PR seems to be correct so far. |
ace826a
to
7e360a1
Compare
Alan, I think I resolved all your concerns with latest commits. I also, squashed them. Meanwhile, thanks fore logic reminder in your #11884 (comment) but according to my understanding it is fully implemented. If something is missing please let me know explicitly. Please note, I'm still awaiting jenkins tests and if something will pop-up I'll resolve it. |
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
Valentin, according to our meeting yesterday, I understand this is ready for another review. If so, then please request it through the GH interface. Before that though, I would suggest you to review some of the comments I left in the code, as I see there are still changes to be applied to this code. In addition, I see a trend of modifying the source code (src/python/*) to be able to test and run unit tests. To me, the code becomes unnecessary more confusing and I would suggest to actually adopt the emulators. If the emulator does not exist, we should create it. If it does not provide the data and/or method that we need, we need to extend it. If needed, that can also be tracked in a different issue. For instance, the Rucio emulator can be found here: https://github.com/dmwm/WMCore/blob/master/src/python/WMQuality/Emulators/RucioClient/MockRucioApi.py we can update the mocked data by updating and running this script: https://github.com/dmwm/WMCore/blob/master/src/python/WMQuality/Emulators/RucioClient/MakeRucioMockFile.py and this is where we are currently mocking Rucio: https://github.com/dmwm/WMCore/blob/master/src/python/WMQuality/Emulators/EmulatedUnitTestCase.py#L58 The last bit of change that you need, is to update the unit test class to actually inherit from I think @d-ylee is going to create a simple documentation for that, given that we discussed basically the same thing yesterday. |
Alan, yes the PR is ready for review, and I did resolved to the best of my ability all requested issues. Said that, even though there are general guidelines I think not everything can be aligned with them. As I pointed out in this PR the test of overwriting the existing tarball requires to write it to another place, that's why additional parameter to source code was added. I did use Rucio Mock APIs but due to limitation of current configuration (it is impossible to pass to config an actual python object) I used similar pattern of adding optional parameter (by the way I've done it in a similar way as we do in MSPileup codebase). This certainly falls into category of adding addition to source code but I can't find any other way to keep it only in test codebase. |
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.
Valentin, if you use the mock.patch
according to what I suggested in my previous reply, there should be no need to pass in a new rucio object instance.
Please review your code and patch objects accordingly, such that there is no need to source code changes to accommodate unit tests.
Alan, thanks for pointers. Honestly, I missed that due to annoying GitHub feature which hide (collapse) long discussions and what you pointed out was hidden when I was reviewed the other changes. I'll take take care of those. And, I'll try to use Said that, before I was awaiting your reply, I was working on implementation of block info via concurrent DBS calls, if you can review this gist I can insert this code already in this PR, since I tested and benchmarked it in local environment. For 463 blocks it takes 9 seconds to get all files and number of events. |
Alan, I unload collapsed GH threads and resolved all issues, only one required removal of obsolete function. Apart from that I addressed all issues already. I'll work on mocking interface now. |
Jenkins results:
|
Jenkins results:
|
Alan, Said that:
|
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.
Valentin, please find my review along the code.
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
self.rucio = None | ||
# define where to look for sandbox tar balls | ||
self.sandboxDir = getattr(self.config.WorkflowUpdater, "sandboxDir") | ||
if not os.path.exists(self.sandboxDir): |
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.
My point is that the workflow spec
parameter is already giving us the location of the tarball (and WMSandbox). So there is no need for us to have this variable defined here.
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
test/python/WMComponent_t/WorkflowUpdater_t/WorkflowUpdaterPoller_t.py
Outdated
Show resolved
Hide resolved
For your gist, I would suggest to have it in the separate PR - especially because now we also have a separated GH issue for that. However, I can already see that your code is inconsistent with the implementation in https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/WMSpec/Steps/Fetchers/PileupFetcher.py, given that your gist does not check whether a file is valid or not. There could be more, but let's wait for the actual review. Side note: for that implementation, I think we could - eventually - start porting other methods used by the microservices, available in this common module: https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/MicroService/Tools/Common.py. Said that, I think we could have a new module created under WMCore/Services/DBS for the pycurl based DBS queries. |
First look at
Yes, I think it would be good idea to put this into separate module. But I suggest to not use DBS Client (DBSReader) since it is sequential codebase, and rather create concurrent functions via |
Jenkins results:
|
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.
@vkuznet please find further review along the code. Before pushing in your upcoming changes, please squash commits accordingly.
src/python/WMComponent/WorkflowUpdater/WorkflowUpdaterPoller.py
Outdated
Show resolved
Hide resolved
Jenkins results:
|
Alan, I addressed your last review feedback and updated the code. The Jenkins are fine. I hope this is the last iteration. Feel free to look-up again. |
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.
@vkuznet Valentin, it's looking good to me. Please squash those commits accordingly and we can merge it.
a051230
to
fc23c16
Compare
Squashing is done. |
Jenkins results:
|
Thanks Valentin |
Fixes #11619
Status
In development
Description
Implement logic for continious update of pileup availability based on #11619 (comment)
Main logic of the algorithm:
FileList
andNumberOfEvents
information, this should be implemented as separate isseupileupconf.json
files within tarballIs it backward compatible (if not, which system it affects?)
YES
Related PRs
External dependencies / deployment changes