Skip to content
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

PAT fix to deal with GTs with added payloads #5053

Merged
merged 1 commit into from Aug 26, 2014

Conversation

Martin-Grunewald
Copy link
Contributor

PAT fix to deal with GTs with added payloads.
This is just in the initialisation step when digesting the default GT 'startup',
which now contains added payloads!

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Martin-Grunewald (Martin Grunewald) for CMSSW_7_2_X.

PAT fix to deal with GTs with added payloads

It involves the following packages:

PhysicsTools/PatAlgos

@nclopezo, @vadler, @cmsbuild, @Degano, @monttj can you please review it and eventually sign? Thanks.
@nhanvtran, @TaiSakuma, @mmarionncern, @schoef this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@ktf
Copy link
Contributor

ktf commented Aug 26, 2014

Bypassing to get this in the IB.

ktf added a commit that referenced this pull request Aug 26, 2014
PAT fix to deal with GTs with added payloads
@ktf ktf merged commit 0a41caf into cms-sw:CMSSW_7_2_X Aug 26, 2014
@davidlange6
Copy link
Contributor

Its fine for the IB, but its clearly the wrong solution - we now have two meanings of auto:startup (unless I misunderstand)

On Aug 26, 2014, at 1:27 PM, Giulio Eulisse notifications@github.com
wrote:

Bypassing to get this in the IB.


Reply to this email directly or view it on GitHub.

@Martin-Grunewald
Copy link
Contributor Author

Hmm, no, not really.

What we have is that the syntax format of the GT could be either just
a simple string ('auto:startup' or "POSTLS172X_V42"), or such a
string plus some appended payload in a comma separated string/list/tuple.

This was already the case previously (see autoCond.py), and GlobalTag.py in
Configuration/AlCa takes care of these variants.

It is just that some code still assumes a globaltag
is just something like the simple string 'startup' or
'PostLS1_V17', without any appended stuff...

@davidlange6
Copy link
Contributor

On Aug 26, 2014, at 2:43 PM, Martin Grunewald notifications@github.com
wrote:

Hmm, no, not really.

What we have is that the syntax format of the GT could be either just
a simple string ('auto:startup' or "POSTLS172X_V42"), or such a
string plus some appended payload in a comma separated string/list/tuple.

This was already the case previously (see autoCond.py), and GlobalTag.py in
Configuration/AlCa takes care of these variants.

It is just that some code still assumes a globaltag
is just something like the simple string 'startup' or
'PostLS1_V17', without any appended stuff…

Yes, I see that. The "fix" appears to be to ignore the appended payloads. Or do I misunderstand?


Reply to this email directly or view it on GitHub.

@Martin-Grunewald
Copy link
Contributor Author

Yes, but this is on purpose as it is just the default initialisation
in the file modified in this PR, based on 'startup' (hardwired),
which should just extract the "START72_V17" (or whatever) part
from 'startup', dropping the "::All".

In the 2 originally failing unit test cases, this gets overwritten by the user
later, with the explicit GT specified in the unit tests: 'PU50ns_POSTLS172_V2',
which does not have appended payloads.
I agree in the long run the PAT file modified in this PR needs additional
changes to deal with user-supplied GTs with payloads, but it seems
no one is using PhysicsTools/PatAlgos/python/tools/cmsswVersionTools.py
this way (yet).

@Martin-Grunewald Martin-Grunewald deleted the PATfix branch August 26, 2014 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants