[py2py3] Migrate WMCore/Cache/* #9818
Conversation
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
| @@ -145,7 +133,7 @@ def handleDictThunk(self, toThunk): | |||
| toThunk = self.checkRecursion(toThunk) | |||
| special = False | |||
| tmpdict = {} | |||
| for k, v in toThunk.iteritems(): | |||
| for k, v in iteritems(toThunk): | |||
| if type(k) == type(int): | |||
There was a problem hiding this comment.
Dear @amaltaro and @todor-ivanov,
I am a bit baffled by this line. it is comparing the type of a key with type(int), which is just type.
It seems that the type of the key is used to prepend a string to the key itself in a temporary dictionary. If the key is an int, we prepend a _i: and shortly aftre if it is a float we prepend _f:.
I would replace if type(k) == type(int): with if isinstance(k, int).
Are my intuitions correct or do we really want to check if a key is of type type?
There was a problem hiding this comment.
That's a trick question. According to google - and my understanding - here we are checking whether k is one of the basic data types or a class definition. The type of those objects is type ...
isinstance seems to accept subclasses as well, while type only compares against the direct object type.
With that said, I have the felling that this code:
elif type(k) == type(float):
...
will never evaluate to True, otherwise the if above (type(int)) would have too.
Here I'm not sure as well, but I think we could replace:
if type(k) == type(int):
by
isinstance(k, type)
Some references that I found:
https://realpython.com/python-metaclasses/#type-and-class
and
https://note.nkmk.me/en/python-type-isinstance/#:~:text=source%3A%20type.py-,With%20isinstance(),an%20instance%20of%20any%20type.
There was a problem hiding this comment.
I would agree with your solution if there was a single check. However here we have something like
for k, v in iteritems(toThunk):
if type(k) == type(int):
tmpdict['_i:%s' % k] = self._thunk(v)
elif type(k) == type(float):
tmpdict['_f:%s' % k] = self._thunk(v)
else:
tmpdict[k] = self._thunk(v)and it seems like the author was trying to put in the dictionary key some information about the value type, but erroneously put a type() in front of the comparison type.
If we use isinstance(k, type) we would match both the int and the float case, wouldn't we?
This being said, I also have the same feeling that those lines now never match True. git blame brought me to 10y ago, when this file was created. If in all these years this change to the keys has been made, should we even change it now?
There was a problem hiding this comment.
I think there is something very much wrong with this code. However, as we discussed over slack, let's keep it as it is, file a new GH issue reporting this problem and move on. In a couple of months from now, we can get back to it.
There was a problem hiding this comment.
I created the issue #9852 related to this.
|
Jenkins results:
|
|
Jenkins results:
|
|
Dear @amaltaro and @todor-ivanov , have a look at last commit's changes. - elif isinstance(toThunk, dict):
+ elif type(toThunk) is dict:This fixes a lot of regressions found by jenkins in a previous commit, for example WMCore_t.WorkQueue_t.Policy_t.Start_t.ResubmitBlock_t/ResubmitBlockTest/testSingleChunksSplit. The incriminated previous commit was something that I considered rather safe, This can, and actually did, break things. Further investigation revealed that when I run the test python setup.py test --buildBotMode=true --reallyDeleteMyDatabaseAfterEveryTest=true --testCertainPath=test/python/WMCore_t/WorkQueue_t/Policy_t/Start_t/ResubmitBlock_t.pyWith this version of JSONThunker (which raises an exception to catch the stdout log) # src/python/WMCore/Wrappers/JsonWrapper/JSONThunker.py
# L 250
elif type(toThunk) is dict:
print("CHECKPOINT", type(toThunk))
temp = self.handleDictThunk(toThunk)
raise Exception("dummy")
return self.handleDictThunk(toThunk)we get While if we run it with # src/python/WMCore/Wrappers/JsonWrapper/JSONThunker.py
# L 250
elif isinstance(toThunk, dict):
print("CHECKPOINT", type(toThunk))
temp = self.handleDictThunk(toThunk)
raise Exception("dummy")
return self.handleDictThunk(toThunk)we get instead If I have to guess, it seems that since This change was not suggested by futurize, and after seeing these errors I understand We applied some similar changes ( |
|
Jenkins results:
|
|
@mapellidario Please note that the contributor to this repo is @todor-ivanov, you are tagging the wrong person. @todor is my account and I am not contributing to this repo, so maybe you will want to remove me and not granting permissions. |
|
I fixed the mention to Todor and myself in your comment above, Dario. |
|
Something strange is happening. I added a new commit yo my feature branch, rebased my feature branch on dmwm/master, pushed with If I open https://github.com/mapellidario/WMCore/tree/py2py3-wmconfigcache it says |
mapellidario
force-pushed
the
py2py3-wmconfigcache
branch
from
d16c773 to
0bcfc70
Compare
|
Yes Dario, try rebasing and squashing everything (that does not involve tests), and it might clear up. Honestly, I'm not sure I understand the problem you reported here. |
|
I think that it simply took about 1h for github to register my last force-push event in this PR. All good what turns out to be good. |
WMCore.Cache and its deps have been made compatible with both python2 and python3. This is relative to the PR on github dmwm/WMCore dmwm#9818. A combination of automatic changes (by futurize, full stage 1 and stage2) and manual changes have been applied. 1. Dictionaries - replaced mydict.$operator() with view$operator(mydict) This is a temporary change and will not be needed anymore after dropping python2 support. This is used for performance reasons when running python2 with idioms that are compatibles with python3. Resources: * http://python-future.org/compatible_idioms.html#dictionaries * http://python-future.org/what_else.html#dict - used the guidelines at http://python-future.org/compatible_idioms.html#dict-keys-values-items-as-a-list for the cases in which we want to check if a string is among the **keys** of a dictionary. no need to use viewkeys for performance reasons. we can just use `list(mydict)` or `for key in mydict:` in both py2 and py3. 2. bytes and unicode strings File: JSONThunker.py - In python3 the meaning of `str()` in a return statement changed. The code now behaves in the same way as before, but the names used to check the string type have been updated to python3 nomenclature 3. urllib File: WMConfigCache.py, `urllib.urlopen` changed to `urllib.request.urlopen` by futurize. - `urllib.request.urlopen` behaves as `urllib2.urlopen`, throwing an exception in case of HTTP error, instead of simply returning valid data containing the details of the error. ACHTUNG! This may require changes to code that use `WMCore.Cache.WMConfigCache.ConfigCache.addConfig` - `urllib.request.urlopen` behaves as `urllib2.urlopen`, requiring to specify the `file:` scheme if you want to open a file in a local filesystem. This required changing `WMConfigCache_t` unit tests.
mapellidario
force-pushed
the
py2py3-wmconfigcache
branch
from
0bcfc70 to
8c9be3d
Compare
|
Jenkins results:
|
Fixes #9806
Status
Done
ACHTUNG! Some of the changes in this PR will conflict with #9762. They are likely to be dealt with in #9762, since it has a lower priority and will likely be merged after this PR is done.
Description
Futurize is run on WMCore.Cache.WMConfigCache and the modules that it depends on.
Manual changes are applied to fix what futurize is not able to migrate:
replaced mydict.$operator() with view$operator(mydict)
This is a temporary change and will not be needed anymore
after dropping python2 support. This is used for performance
reasons when running python2 with idioms that are compatibles
with python3.
Resources:
used a
iter*operator only once in a loop in which the dictionary that was being looped over is updatedused the guidelines at
http://python-future.org/compatible_idioms.html#dict-keys-values-items-as-a-list
for the cases in which we want to check if a string is
among the keys of a dictionary. no need to use viewkeys for performance reasons.
we can just use
list(mydict)orfor key in mydict:in both py2 and py3.File: JSONThunker.py
str()in a return statement changed.The code now behaves in the same way as before, but the names
used to check the string type have been updated to python3 nomenclature
File: WMConfigCache.py,
urllib.urlopenchanged tourllib.request.urlopenby futurize.urllib.request.urlopenbehaves asurllib2.urlopen, throwing anexception in case of HTTP error, instead of simply returning valid
data containing the details of the error.
ACHTUNG! This may require changes to code that use
WMCore.Cache.WMConfigCache.ConfigCache.addConfigurllib.request.urlopenbehaves asurllib2.urlopen, requiring tospecify the
file:scheme if you want to open a file in a localfilesystem. This required changing
WMConfigCache_tunit tests.Migration plan
step1
step2
step 3
step 4
step5
step 6
step 7
step 8
Is it backward compatible (if not, which system it affects?)
yes
External dependencies / deployment changes
requires python-future.org