[py2py3] Fix pre-python2.6 idiom: use "isinstance()" instead of "type() == "

[mapellidario]

Fixes #10171

Status

Ready

Related PR

This PR is independent form other PRs

Description

Run futurize -f idioms on src/python.

Manual changes

• type([]) changed to list
• type(1) changed to int
• type.ListType changed to list

Changes involving type(dict), which can cause problems (despite no problem has been caught by jenkins CI)

• src/python/WMCore/WMSpec/Steps/BuildTools.py
• src/python/WMCore/WMSpec/ConfigSectionTree.py

What has been kept back and not included in this PR

• JSONThunker had some problems in the past with this fixers. Moreover, JSONThunker - strange dictionary key type comparison #9852 is still open.
• WMException: this change caused a dramatic change in behavior that is being addressed in separate PRs, such as [py2py3] Fix potential bug in WMException #10174, [py2py3] Fix potential bug in WMException - again #10187, [py2py3] Fix potential bug in WMException - No modernization of WMException #10189

Is it backward compatible (if not, which system it affects?)

Yes, it should be. any possibly breaking changes will be reported here.

External dependencies / deployment changes

This does not rely on third party libraries

[mapellidario]

After private discussion with Alan, we decided that WMException would be left out of this PR, since we are dealing with it in #10174, #10187, #10189. (The PR opening comment has been updated accordingly)

[cmsdmwmbot]

Jenkins results:

• Unit tests: succeeded
• Pylint check: failed
  • 101 warnings and errors that must be fixed
  • 5 warnings
  • 257 comments to review
• Pylint py3k check: succeeded
  • 0 errors and warnings that should be fixed
  • 0 warnings
  • 0 comments to review
• Pycodestyle check: succeeded
  • 491 comments to review
• Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/10996/artifact/artifacts/PullRequestReport.html

[amaltaro]

Dario, this PR looks pretty good to me. I have requested only one change, and I have a couple of concerns along the code as well.

[cmsdmwmbot]

Jenkins results:

• Unit tests: succeeded
• Pylint check: failed
  • 101 warnings and errors that must be fixed
  • 5 warnings
  • 255 comments to review
• Pylint py3k check: succeeded
  • 0 errors and warnings that should be fixed
  • 0 warnings
  • 0 comments to review
• Pycodestyle check: succeeded
  • 489 comments to review
• Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11044/artifact/artifacts/PullRequestReport.html

[amaltaro]

Dario, thanks for the follow up.
It's isn't clear to me whether further changes should be made here. Please let me know if you consider it ready to go or if you want to make further changes to the ConfigSectionTree module. Thanks

[mapellidario]

I know I have not been too clear in answering to your comment.

In any case: I can create a new issue about "ConfigSectionTree" and we move this discussion there, so that it is not lost. The rest of the changes here look good to me and I suggest to merge this PR if the rest looks good to you.

[EDIT]: created #10218, moving this discussion over there

Problem in ConfigSectionTree

Before #10103 we had

def formatNative(value):
    if isinstance(value, int):
        return value
    if isinstance(value, float):
        return value
    if isinstance(value, list):
        return value
    if isinstance(value, dict):
        return value # this was actually "return dict", but it is outside of our scope now
    else:
        return format(value)

def format(value):
    if isinstance(value, str):
        value = "\'%s\'" % value
    return str(value)

Since we run this code in py2 only, only py2-str would get surrounded in \', not unicode strings, and the return type of this function would be py2-str (i.e. bytes string)

In #10103 we introduced from builtins import str. This means that right now, when we run the code in python2, py2-str are not surrounded by \', instead unicode strings are, and the return type of the function is unicode, not bytes.

Implications

Is this change a real problem? I am not sure. formatNative seems to be used only in WMCore/WMSpec/WMTask.WMTaskHelper.getGeneratorSettings in

WMCore/src/python/WMCore/WMSpec/WMTask.py

Line 631 in 08c1eea

tempArgs = confValues.pythoniseDict(sections=False)

Which is then used in WMCore/JobSplitting/Generators/GeneratorManager.GeneratorManager and WMCore/JobSplitting/Generators/GeneratorFacotory.GeneratorFactory.makeGenerators.

How to proceed

We have a couple of options and I am not sure which is the best, since it is a bit hard for me to graps how the "pythonisedDict" is used in the Generators.

1. Leave it as is (surround unicode and return unicode)

and investigate in testbed the impact of this problem.

(This may not be the best option)

2. Surround both unicode and bytes, return unicode

We could change

- from builtins import str, object
+ from builtins import str, bytes, object

def format(value):
-    if isinstance(value, str):
+    if isinstance(value, (str, bytes)):
        value = "\'%s\'" % value
+    return str(value)

(This is a nice option)

3. Make ConfigSectionTree behave the same as before #10103 (surround bytes and return bytes)

We could change

- from builtins import str, object
+ from builtins import str, bytes, object

def format(value):
-    if isinstance(value, str):
+    if isinstance(value, bytes):
        value = "\'%s\'" % value
-    return str(value)
+    return bytes(value)

This may not be what we want, though, especially in python3.

4. Try the "native string" approach

This is inspired by the Fedora's Conservative Py3 Porting Guide https://portingguide.readthedocs.io/en/latest/strings.html#the-native-string

The idea is: often, when we use str in py2, we are just consistently using str and we may be better off to just forget about unicode,bytes and other amenities and just keep using str in both py2 and py3, even though it means different things.

when run from py2 interpreter:

• surround bytes, return bytes
  when run from py3 interpreter:
• surround unicode, return unicode

(This is a nice option)

[cmsdmwmbot]

Jenkins results:

• Unit tests: succeeded
• Pylint check: failed
  • 101 warnings and errors that must be fixed
  • 5 warnings
  • 255 comments to review
• Pylint py3k check: succeeded
  • 0 errors and warnings that should be fixed
  • 0 warnings
  • 0 comments to review
• Pycodestyle check: succeeded
  • 489 comments to review
• Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11046/artifact/artifacts/PullRequestReport.html

[amaltaro]

Thanks Dario. We better discuss that possible issue in the new issue you created. You further commits look good to me, can you please squash them in the first one? Thanks

[cmsdmwmbot]

Jenkins results:

• Unit tests: succeeded
• Pylint check: failed
  • 101 warnings and errors that must be fixed
  • 5 warnings
  • 255 comments to review
• Pylint py3k check: succeeded
  • 0 errors and warnings that should be fixed
  • 0 warnings
  • 0 comments to review
• Pycodestyle check: succeeded
  • 489 comments to review
• Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11048/artifact/artifacts/PullRequestReport.html