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

Fix edmPythonSearch to also look in modules from process.load(...) #9649

Merged
merged 2 commits into from Jun 20, 2015

Conversation

mark-grimes
Copy link

Currently the edmPythonSearch tool doesn't include modules imported with process.load( <module> ), which makes it almost useless. This adds a custom override of the modulefinder opcode scanner which searches for process.load(...) statements, as well as the normal import <module> type statements.

It should work regardless of the name of the Process object, and regardless of how FWCore.ParameterSet.Config was imported (e.g. import FWCore.ParameterSet.Config or import FWCore.ParameterSet.Config as foo)

As an example, step3 of workflow 1306.0 (and probably all others) uses "process.load" to import Configuration.StandardSequences.Services_cff, which (as an arbitrary example) contains the comment:

#an "intermediate layer" remains, just in case somebody is using it...

Before this pull request

edmPythonSearch "just in case somebody" 1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15+MINIAODMCUP15/step3_RAW2DIGI_L1Reco_RECO_EI_VALIDATION_DQM.py

yields no results at all.

After this pull request it gives:

#an "intermediate layer" remains, just in case somebody is using it...
Configuration.StandardSequences.Services_cff (line: 10)
From 1306.0.step3_RAW2DIGI_L1Reco_RECO_EI_VALIDATION_DQM

Examples are easy to find where patterns aren't found as the code stands before this pull request.
Pretty sure this only affects the edmPythonSearch tool, I can't see how any reconstruction/simulation code would be affected.
Most of this code was based on the modulefinder source (https://hg.python.org/cpython/file/2.7/Lib/modulefinder.py) and some useful information at http://security.coverity.com/blog/2014/Nov/understanding-python-bytecode.html.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mark-grimes (Mark Grimes) for CMSSW_7_5_X.

Fix edmPythonSearch to also look in modules from process.load(...)

It involves the following packages:

FWCore/ParameterSet

@cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @wddgit, @wmtan 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@Dr15Jones
Copy link
Contributor

Please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@mark-grimes
Copy link
Author

Just found some examples where it still misses things, e.g.

edmPythonSearch l1CaloScales 1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15+MINIAODMCUP15/SingleMuPt1_pythia8_cfi_GEN_SIM.py

Comes up with nothing when it should have (at least)

__main__ ->
SLHCUpgradeSimulations.Configuration.postLS1Customs (customisePostLS1) ->
L1Trigger.L1TCommon.customsPostLS1 (customiseSimL1EmulatorForPostLS1_25ns) ->
L1Trigger.L1TCommon.customsPostLS1 (customiseSimL1EmulatorForStage1) ->
L1Trigger.L1TCalorimeter.L1TCaloStage1_cff ->
line 9:from L1TriggerConfig.L1ScalesProducers.l1CaloScales_cfi import l1CaloScales
line 10:l1CaloScales.L1HfRingThresholds = cms.vdouble(0.0, 24.0, 28.0, 32.0, 36.0, 40.0, 44.0, 48.0)
line 12:## l1CaloScales.L1HtMissThresholds = cms.vdouble(
line 28:l1CaloScales.L1HtMissThresholds = cms.vdouble(

Also realised that it doesn't allow for conditional process.load, i.e. it will (should) give results for every process.load in a file regardless of whether the logic actually calls them (although in the example above this is broken anyway). I don't see any way around this without a massive rewrite and not using modulefinder, which I think is not worth the effort. It's better to return slightly too many results than to miss some right? Once I fix the problem above that is.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9649/31/summary.html

There are some workflows for which there are errors in the baseline:
25202.0 step 2
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

else:
code = code[1:]

def removeRecursiveLoops( node, verbose=False, currentStack=None ) :
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, one can also get it to report recursive loops (like the ones mentioned in my comments) by changing verbose to True. I defaulted it to False because it can throw up a lot of erroneous loops, and I thought general users would worry without reason.

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Jun 20, 2015
Fix edmPythonSearch to also look in modules from process.load(...)
@cmsbuild cmsbuild merged commit 800bd18 into cms-sw:CMSSW_7_5_X Jun 20, 2015
@mark-grimes mark-grimes deleted the fixPythonSearch branch June 22, 2015 08:19
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