cfipython file changes in 14_1_0_pre4 (compared to pre3) lead to failures in ConfDb parsing #45087
Comments
|
assign core |
|
New categories assigned: core @Dr15Jones,@makortel,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks |
|
cms-bot internal usage |
|
A new Issue was created by @Martin-Grunewald. @antoniovilela, @rappoccio, @Dr15Jones, @smuzaffar, @sextonkennedy, @makortel can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
|
The generated cfipython files no longer seem to be self-contained or fully-defined python files: pre4: while there is no error in pre3: I hope this can be fixed! |
|
The following makes it work: cat apvModeFilter_cfi.py ie, with a fully qualified import path name, |
|
presumably a fix is needed here then: (changes introduced at #44363) |
The problem is ConfDB is not properly following the python rules for doing an import. If one were going to use a subset of What I believe needs to be changed is this line in ConfDB which should be like the line two above it @Martin-Grunewald how do I checkout and run ConfDB to see if that change is all that is needed? |
|
Sorry, your reply is confusing: the py file is not even working with Could you please clarify? |
|
Can we make |
|
So I did |
|
This also works |
|
What I mean is that this should work: which no longer works in pre4 but does in pre3. |
I'm afraid I disagree. What is intended to work is python3 -m Alignment.CommonAlignment.apvModeFilter_cfii.e. the exact syntax you are required to use in an |
|
That's great, so you removed functionality which worked in the past and tell me that is how it's supposed to be? Seriously? |
|
@Martin-Grunewald Could you comment if the |
|
So with the following changes to @@ -698,12 +705,8 @@ class ConfdbLoadParamsfromConfigs:
thesubsystem = thesubsystempackage.split('/')[0]
thepackage = thesubsystempackage.split('/')[1]
- importcommand = ""
- if(usepythonsubdir == True):
- importcommand = "import " + thesubsystem + "." + thepackage + "." + thecomponent
- else:
- importcommand = "import " + thecomponent
- sys.path.append(pydir)
+ importcommand = "import " + thesubsystem + "." + thepackage + "." + thecomponent
+ #sys.path.append(pydir)@@ -715,11 +718,7 @@ class ConfdbLoadParamsfromConfigs:
self.VerbosePrint(importcommand,1)
exec(importcommand)
# Now create a process and construct the command to extend it with the py-cfi
- theextend = ""
- if(usepythonsubdir == True):
- theextend = "process.extend(" + thesubsystem + "." + thepackage + "." + thecomponent + ")"
- else:
- theextend = "process.extend(" + thecomponent + ")"
+ theextend = "process.extend(" + thesubsystem + "." + thepackage + "." + thecomponent + ")"
self.VerbosePrint(theextend,1)
eval(theextend)To test this, I made additional changes to It would be good if there was an actual way to test the parsing without having to also use the DB. |
|
Thanks a lot @Dr15Jones for looking at this and providing the fixes. Given the above change is a simplification, I now wonder why the original had been coded cumbersome like that with the if-then-else alternative. And then I worry about backward compatibility, ie, parsing an old (say 14_0) release with the changes above is expected to yield the same results? |
I put it on lxplus at ~cdj/public/ConfdbLoadParamsFromConfigsPy3.py . Let me know if that isn't actually public.
It would be nice if the code were more factorized so that the parsing part could be tested completely independent of the part that reads/writes to the DB. That could allow full testing of the parser in CMSSW possibly.
As long as there were not two identically named files in the same package (one under |
|
Thanks a lot for the file (found it under |
|
With Increasing verbosity level to |
|
The generated cfi file has: |
|
@Martin-Grunewald yeah, to make the I looked at How do you want to proceed? |
|
Note, you could easily blacklist |
|
A trivial workaround would be to change line 1204 from parametervalue = pval.value()to if hasattr(pval, "value"):
parametervalue = pval.value()
else:
parametervalue = "" |
|
Another option would be to remove To do that line 328-329 could probably be changed from for temptype, tempname in temptuple:
self.paramtypedict[temptype] = tempnameto for temptype, tempname in temptuple:
if temptype != 'EventID':
self.paramtypedict[temptype] = tempname |
|
I've extended the other python parameters to have a |
|
Thanks for this, for now/to test, I blacklisted in the code the files. Apart from the duplicate file (cfipython and python) you found, I see that the parsing makes changes to the three following modules (comparing pre3 to pre4 and migrating the HLT menu from pre3 to pre4): which is UNexpected. It seems for these three modules, there are py files with the same name in cfipython and python also. |
The precedence of I'm planning on opening an issue to make it a 'build' error if the same file name appears in |
|
|
In these cases the python variants are not the ones we want as they do not specify all parameters. But yes if they are made consistent or better, removed, that's great. Note this migration only checks modules actually used in the HLT... |
|
The duplicate _cfi.py files were removed in #45112. |
|
Have all the issues been resolved now? (i.e. can this issue be closed?) |
|
From my side, yes! Thanks all for support! |
|
Thanks! |
|
+core |
|
@cmsbuild, please close |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
This issue is fully signed and ready to be closed. |
Between pre3 and pre4 of 14_1_0, there is the following change in fillDescriptions-generated cfipython files, for example:
pre3:
while in pre4:
The pre4 version breaks ConfDb parsing with the error message:
Clearly this is a problem.
It seems the issue is with
from .APVModeFilter import APVModeFilter, the leading '.' syntax.Can it be reverted or done differently?
For example, defining the
parent packageexplicitly in the py file, or giving a full path in the import statement?The text was updated successfully, but these errors were encountered: