Adjust scopes in Rucio wrapper and MSPileupTasks to support proper data placement of custom containers #11938
Conversation
|
Jenkins results:
|
vkuznet
changed the title
|
@vkuznet Valentin, even though you haven't requested for a review yet, I wanted to mention 2 points:
|
|
Jenkins results:
|
|
Jenkins results:
|
Alan, I don't see need for docstring update in isContainer since it already has |
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
| dids = [{'scope': scope, 'name': did} for did in dids] | ||
| newDIDs = [] | ||
| for did in dids: | ||
| if '#' in did: |
There was a problem hiding this comment.
Is there any reason to implement this check for block here? I would simply write it as we discussed last week, e.g.:
dids = [{'scope': 'cms', 'name': did} for did in dids]
and add a comment that, if DIDs need to be attached against a different scope, further development is needed.
There was a problem hiding this comment.
I think it is correct check since blocks must be always be in CMS scope while datasets (rucio container) may not. Since we should acknowledge given scope to this function I provided correct behavior, i.e. in input scope can be either cms or custom one and we should follow what is provided. Your suggestion will break this.
There was a problem hiding this comment.
We don't attach container to containers. So setting cms scope for those dids is fine.
There was a problem hiding this comment.
Alan, please scan Rucio.py code to see where attachDIDs is used, e.g. createBlock, createReplicas APIs. The latter have specific scope input parameter, therefore I do not know how it will affect the other use-cases. I provided safest and correct way to follow the given scope and only make exception for dids which are CMS blocks.
There was a problem hiding this comment.
I really would like to avoid checking for a block by verifying the existence of the # symbol, which is a fragile logic. That's the reason I am suggesting to simply set it to cms instead.
createBlock --> attaches blocks
createReplicas --> attaches files/replicas. So hardwiring it to cms is as good as the current logic, but cleaner.
There was a problem hiding this comment.
I'll not argue anymore, but I made my point. Said that, the code now has changed to use your proposal and I added proper comment. Please refresh the PR to see last commit: 53fcc3e
|
Jenkins results:
|
|
Jenkins results:
|
vkuznet
force-pushed
the
fix-issue-11920-v2
branch
from
53fcc3e
to
8658ead
Compare
|
commits are squashed |
|
Jenkins results:
|
|
It looks like you missed the PR description. I just updated it accordingly. |
Fixes #11937.
Status
ready
Description
Fix scope usage in rucio client APIs. In details:
cmsscopeIn addition, further fixes to MSPileupTasks, such as:
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
#11921
External dependencies / deployment changes