Copy DQM ROOT file to EOS and register entry in DQMUpload executor #11015
Conversation
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
khurtado
force-pushed
the
dqmupload1
branch
2 times, most recently
from
b8e5661
to
2e3d149
Compare
|
Jenkins results:
|
|
Jenkins results:
|
- Copy DQM root files to EOS - Register files to new DQMGUI via http post method - If registration fails, delete EOS files upload
|
@amaltaro I think this *almost is ready for review. What is pending before the code review is getting a new permanent EOS area, managed by WMCore/production, rather than the unmerged area. Who should we contact for that? |
khurtado
force-pushed
the
dqmupload1
branch
2 times, most recently
from
ed3384c
to
49b785b
Compare
|
Jenkins results:
|
|
Jenkins results:
|
khurtado
force-pushed
the
dqmupload1
branch
2 times, most recently
from
4039951
to
7754191
Compare
- Fix indentation - Assume single register URL - Retry registration - Use self.job directly
|
Jenkins results:
|
|
@amaltaro I think I addressed most of the comments and left comments on the ones I didn't |
|
Jenkins results:
|
|
Jenkins results:
|
There was a problem hiding this comment.
Kenyi, sorry for the delay reviewing this. Next time, please refresh the review request through GH such that we can properly filter it.
While reviewing it, it just occurred to me that T0 also uses this code. So we need to verify whether T0 would need to use this same mechanism; or whether we need to work on workflow configuration that could enable/disable this new DQM feature. Can you please follow this up with the DQM team?
| self.retryCount = 3 | ||
| self.registerLFNBase = '/store/unmerged/DQMGUI' | ||
| self.registerEOSPrefix = '/eos/cms' | ||
| self.registerURL = 'https://cmsweb-testbed.cern.ch/dqm/offline-test-new/api/v1/register' |
There was a problem hiding this comment.
I think so, and they are likely workflow dependent.
What I am trying to say is whether we could use the spec DQMUploadUrl url to decide which DQM Register url to use? Otherwise, I fear that we might have to create yet another StdSpecs parameter for all our workflow specs. A third option would be to hard code it, but I would only be okay with it it we are likely not going to change this url for the coming couple of years.
| """ | ||
| # If lfn start with '/', make it relative to it | ||
| lfnFile = os.path.relpath(analysisFile.lfn, start='/') | ||
| lfn = os.path.join(self.registerLFNBase, lfnFile) |
There was a problem hiding this comment.
and this is a surprise to me as well!
There was a problem hiding this comment.
@khurtado Kenyi, here goes a fresh review of the current changes. In addition to the comments made along the code, I'd like to mention:
- Please update the initial PR description.
- I think it would be extremely useful to have a short summary of what DQMUpload module does at the top of the module, including all the steps it takes (since it no longer does a simple upload of the root file).
- We also need to clarify whether T0 workflows will have to follow the same new design (have you heard anything from the DQM team?)
- Did we agree on a final map of urls? If so, it needs to be implemented as well.
| self.retryCount = 3 | ||
| self.registerLFNBase = '/store/unmerged/DQMGUI' | ||
| self.registerEOSPrefix = '/eos/cms' | ||
| self.registerURL = 'https://cmsweb-testbed.cern.ch/dqm/offline-test-new/api/v1/register' |
There was a problem hiding this comment.
Should we create a method to get the register url (from the map that we discussed in the GH issue)?
| self.httpPost(os.path.join(stepLocation, | ||
| os.path.basename(analysisFile.fileName))) | ||
| # Upload to EOS and register (new method) | ||
| self.uploadToEOSAndRegister(step, stepLocation, analysisFile) |
There was a problem hiding this comment.
In terms of actions, perhaps we could have two different methods here:
- uploading the root file to EOS
- registering the root file metadata in the new DQM Gui
what do you think?
| """ | ||
| # If lfn start with '/', make it relative to it | ||
| lfnFile = os.path.relpath(analysisFile.lfn, start='/') | ||
| lfn = os.path.join(self.registerLFNBase, lfnFile) |
There was a problem hiding this comment.
Kenyi, coming back to this, what format is analysisFile.lfn supposed to have? Can you please give me an example (or maybe two, one for TaskChain and one for StepChain)?
This relpath has a behaviour that could possibly cause problems, see:
In [8]: os.path.relpath('test', start='/')
Out[8]: 'Users/amaltaro/Pycharm/cmsdist/test'
In [9]: os.path.relpath('/test', start='/')
Out[9]: 'test'
|
|
||
| def _register(self, registerURL, args): | ||
| """ | ||
| POST request to register URL |
There was a problem hiding this comment.
Maybe we could have something like def _getHttpsOpener with the initial common code? It it becomes too ugly, then we keep these into 2 different methods.
|
Jenkins results:
|
|
Hi @amaltaro , to answer your questions:
|
|
Jenkins results:
|
The initial PR description at the very top, here: #11015 (comment) |
Sounds good! I just extended that bullet a little bit. |
|
Jenkins results:
|
- Use upload.URL -> registerURL mapping - Address PR comments
@amaltaro Thanks! I just made that last change. |
|
Jenkins results:
|
|
Can one of the admins verify this patch? |
Fixes #10287
Status
New changes being tested
Description
This implements the new DQM Gui method described in #10287
Is it backward compatible (if not, which system it affects?)
YES