-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Better era imports #16001
Better era imports #16001
Conversation
A new Pull Request was created by @kpedro88 (Kevin Pedro) for CMSSW_8_1_X. It involves the following packages: CalibCalorimetry/HcalPlugins @ghellwig, @lveldere, @ianna, @cerminar, @rekovic, @vanbesien, @civanch, @monttj, @cmsbuild, @davidlange6, @smuzaffar, @Dr15Jones, @cvuosalo, @mdhildreth, @slava77, @mommsen, @mmusich, @mulhearn, @ssekmen, @emeschi, @dmitrijus, @franzoni can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
@cmsbuild please test |
The tests are being triggered in jenkins. |
Since the bot ran out of tags: |
@kpedro88 Please check expanded configs in several standard workflows to confirm that your changes provide identical results. |
@slava77 I saw that all workflows in the short matrix ran to completion with no errors. I was planning to rely on the comparisons from the bot. Is there a way that I can run the comparison generation script myself? |
comparisons are not enough. We post a link to Reco tools in every RECO/AT meeting. |
Ah, I see what you mean now. I can write a script to call edmConfigDump on everything and diff vs. a clean IB. |
I can take care of this (after this PR gets merged). |
@makortel thanks! (the second commit in this PR shows which files have that usage) |
On 9/27/16 1:32 PM, Kevin Pedro wrote:
Nice.
|
+1 |
+1 |
Comparisons look good to me at a glance. Again, would prefer to merge quickly to avoid conflicts popping up... (this will also make it quicker to test any other PRs that modify Era-related code) |
I agree. All - any additional checks needed beyond what can be done over the next few hours (eg, as to merge by the next IB)?
|
please comment if not ok even if now merged |
(this is probably the wrong place to comment but anyway) Interestingly, in CMSSW_8_1_X_2016-09-28-2300 after a single edit in Are we missing something (additional) here? |
i guess it will change once we have a full build of the IB instead of a patch. we’ll try to trigger a full build today
|
Ok, so the list needs a full build (I bit of suspected that), thanks. |
@makortel
|
On 9/30/16 12:52 PM, Kevin Pedro wrote:
nice
|
@kpedro88 Nice indeed :) I'll attack now on the remaining imports in tracking. |
This PR addresses ~90% of the instances of importing
Configuration.StandardSequences.Eras
, replacing those imports with the modifier-specific cff files now available. Most of the edits were made automatically using a Python script, with a few corner cases fixed by hand afterward.Remaining types of Era imports still to be dealt with:
getattr(eras,era)
in a general loop.cmsRun
configs generated bycmsDriver
use the import to initialize thecms.Process
object. (Most of the tracked configs are stored intest
directories, so aren't picked up by checkdeps.)import Configuration.StandardSequences.Eras as eras
.These special cases of imports should probably be addressed by someone who understands the expected function of the code.
This PR is going to touch a huge number of categories, but I would appreciate merging it quickly once it's confirmed not to cause any regressions, in order to avoid never-ending rebases. We should start enforcing the new way of importing specific modifiers in open PRs. I tested this PR using the short matrix in the latest IB and all tests passed.
attn: @davidlange6, @Dr15Jones, @slava77