Replace imp module by importlib #11530

vkuznet · 2023-04-04T12:59:12Z

Fixes #11434

Status

ready

Description

Replace import imp occurrences with import importlib

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

MAYBE

Related PRs

External dependencies / deployment changes

cmsdmwmbot · 2023-04-04T13:13:30Z

Jenkins results:

Python3 Unit tests: failed
- 17 new failures
Python3 Pylint check: failed
- 75 warnings and errors that must be fixed
- 6 warnings
- 173 comments to review
Pylint py3k check: failed
- 10 errors and warnings that should be fixed
- 2 warnings
Pycodestyle check: succeeded
- 146 comments to review

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

cmsdmwmbot · 2023-04-04T14:38:12Z

Jenkins results:

Python3 Unit tests: failed
- 2 new failures
- 2 changes in unstable tests
Python3 Pylint check: failed
- 50 warnings and errors that must be fixed
- 6 warnings
- 169 comments to review
Pylint py3k check: failed
- 10 errors and warnings that should be fixed
- 2 warnings
Pycodestyle check: succeeded
- 143 comments to review

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

cmsdmwmbot · 2023-04-04T15:08:39Z

Jenkins results:

Python3 Unit tests: failed
- 2 new failures
- 1 tests no longer failing
Python3 Pylint check: failed
- 47 warnings and errors that must be fixed
- 6 warnings
- 170 comments to review
Pylint py3k check: failed
- 10 errors and warnings that should be fixed
- 2 warnings
Pycodestyle check: succeeded
- 145 comments to review

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

cmsdmwmbot · 2023-04-04T16:38:30Z

Jenkins results:

Python3 Unit tests: succeeded
- 1 tests no longer failing
- 1 changes in unstable tests
Python3 Pylint check: failed
- 47 warnings and errors that must be fixed
- 6 warnings
- 170 comments to review
Pylint py3k check: failed
- 10 errors and warnings that should be fixed
- 2 warnings
Pycodestyle check: succeeded
- 145 comments to review

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

amaltaro

@vkuznet Valentin, the python3 documentation suggests different replacements for imp.find_module and imp.load_module, see:
https://docs.python.org/3/library/imp.html#imp.load_module

Can you please clarify why you implemented these with importlib.machinery instead of importlib.utils?

vkuznet · 2023-04-05T12:13:13Z

Alan, I made changes as close to original implementation as possible. Here are different ways of doing module loading:

using img library

    cfgBaseName = os.path.basename(configPath).replace(".py", "")
    cfgDirName = os.path.dirname(configPath)
    modPath = imp.find_module(cfgBaseName, [cfgDirName])
    loadedConfig = imp.load_module(cfgBaseName, modPath[0],
                                   modPath[1], modPath[2])

using importlib.machinery implementation

    cfgBaseName = os.path.basename(configPath).replace(".py", "")
    cfgDirName = os.path.dirname(configPath)
    modSpecs = importlib.machinery.PathFinder().find_spec(cfgBaseName, [cfgDirName])
    module = modSpecs.loader.load_module()

using importlib.util implementation

    cfgBaseName = os.path.basename(configPath).replace(".py", "")
    spec = importlib.util.spec_from_file_location(cfgBaseName, configPath)
    module = importlib.util.module_from_spec(spec)

The Python documentation does not explicitly says which method is better (even though it provides examples of importlib.util) and both importlib.machiner and importlib.util are appropriate methods to load modules. The former is more abstract and flexible though.

todor-ivanov

Hi @vkuznet
Thanks for this PR. I was about to ask the same question as Alan did. And to me the third implementation you were suggesting looks the best. Talking about this one:

using importlib.util implementation

   cfgBaseName = os.path.basename(configPath).replace(".py", "")
   spec = importlib.util.spec_from_file_location(cfgBaseName, configPath)
   module = importlib.util.module_from_spec(spec)

vkuznet · 2023-04-10T12:57:43Z

@amaltaro , @todor-ivanov could you please clarify how we should move forward with this PR. Neither of you explicitly said let's change the fix to use importlib.utils instead of importlib.machinery. If you feel this way please say it explicitly, otherwise you left me guessing about your decision, or if you ok with current implementation let's merge it.

amaltaro · 2023-04-11T01:31:41Z

@vkuznet Valentin, my preference goes towards importlib.util, but given that you have already made all the relevant changes, including to the unit tests, I feel like it's not worth it changing the implementation/module to use.

I do have a concern though, instead of importing the whole importlib, should we actually import only what's going to be used, thus:

from importlib import machinery

?

vkuznet · 2023-04-12T19:05:00Z

Alan, the import lib has only one machiner.py module therefore

from importlib import machinery

is equivalent to

import importlib.machiner

In both cases they provide identical content, e.g.:

>>> import importlib.machinery
>>> help(import.machinery)
Help on module importlib.machinery in importlib:

NAME
    importlib.machinery - The machinery of importlib: finders, loaders, hooks, etc.

MODULE REFERENCE
    https://docs.python.org/3.11/library/importlib.machinery.html

    The following documentation is automatically generated from the Python
    source files.  It may be incomplete, incorrect or include features that
    are considered implementation detail and may vary between Python
    implementations.  When in doubt, consult the module reference at the
    location listed above.

FUNCTIONS
    all_suffixes()
        Returns a list of all recognized module suffixes for this process

DATA
    BYTECODE_SUFFIXES = ['.pyc']
    DEBUG_BYTECODE_SUFFIXES = ['.pyc']
    EXTENSION_SUFFIXES = ['.cpython-311-darwin.so', '.abi3.so', '.so']
    OPTIMIZED_BYTECODE_SUFFIXES = ['.pyc']
    SOURCE_SUFFIXES = ['.py']

FILE
    /opt/local/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/importlib/machinery.py

vs

>>> from importlib import machinery
>>> help(importlib.machinery)
Help on module importlib.machinery in importlib:

NAME
    importlib.machinery - The machinery of importlib: finders, loaders, hooks, etc.

MODULE REFERENCE
    https://docs.python.org/3.11/library/importlib.machinery.html

    The following documentation is automatically generated from the Python
    source files.  It may be incomplete, incorrect or include features that
    are considered implementation detail and may vary between Python
    implementations.  When in doubt, consult the module reference at the
    location listed above.

FUNCTIONS
    all_suffixes()
        Returns a list of all recognized module suffixes for this process

DATA
    BYTECODE_SUFFIXES = ['.pyc']
    DEBUG_BYTECODE_SUFFIXES = ['.pyc']
    EXTENSION_SUFFIXES = ['.cpython-311-darwin.so', '.abi3.so', '.so']
    OPTIMIZED_BYTECODE_SUFFIXES = ['.pyc']
    SOURCE_SUFFIXES = ['.py']

FILE
    /opt/local/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/importlib/machinery.py

amaltaro · 2023-04-12T21:33:54Z

Sorry, I guess I wasn't clear enough in my previous message. What I wanted to say is that, if only the machinery module is used in a WMCore module, ideally we should change the import from:

import importlib

to

from importlib import machinery

From what I read, the lack of this change will not affect memory footprint though. So we can just leave it for another day as well.

vkuznet · 2023-04-13T13:08:06Z

Alan, I rather prefer to keep the code this way as it clears what it is doing in placed which have been changed, i.e. the prefix of importlib.machiner what makes it clear.

amaltaro

Sounds good to me, Valentin!
Given that we are addressing the final bits for an upcoming production deployment, let's merge it only after upgrading services in CMSWEB production.

vkuznet requested a review from amaltaro April 4, 2023 12:59

vkuznet self-assigned this Apr 4, 2023

vkuznet removed the request for review from amaltaro April 4, 2023 13:28

vkuznet force-pushed the fix-issue-11434 branch from 73fe4b5 to b27544e Compare April 4, 2023 14:27

vkuznet force-pushed the fix-issue-11434 branch from b27544e to 97b8a76 Compare April 4, 2023 14:59

vkuznet added 2 commits April 4, 2023 12:20

Replace imp by importlib

9667aac

Replace imp by importlib in unit tests

51cee11

vkuznet force-pushed the fix-issue-11434 branch from 97b8a76 to 51cee11 Compare April 4, 2023 16:22

vkuznet requested review from amaltaro and todor-ivanov April 4, 2023 16:40

amaltaro reviewed Apr 4, 2023

View reviewed changes

todor-ivanov reviewed Apr 6, 2023

View reviewed changes

amaltaro self-requested a review April 13, 2023 13:18

amaltaro approved these changes Apr 13, 2023

View reviewed changes

amaltaro merged commit 0f27526 into dmwm:master Apr 21, 2023

todor-ivanov mentioned this pull request May 5, 2023

Broken usage of importlib #11585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace imp module by importlib #11530

Replace imp module by importlib #11530

vkuznet commented Apr 4, 2023

cmsdmwmbot commented Apr 4, 2023

cmsdmwmbot commented Apr 4, 2023

cmsdmwmbot commented Apr 4, 2023

cmsdmwmbot commented Apr 4, 2023

amaltaro left a comment

vkuznet commented Apr 5, 2023

todor-ivanov left a comment

vkuznet commented Apr 10, 2023

amaltaro commented Apr 11, 2023

vkuznet commented Apr 12, 2023

amaltaro commented Apr 12, 2023

vkuznet commented Apr 13, 2023

amaltaro left a comment

Replace imp module by importlib #11530

Replace imp module by importlib #11530

Conversation

vkuznet commented Apr 4, 2023

Status

Description

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

Related PRs

External dependencies / deployment changes

cmsdmwmbot commented Apr 4, 2023

cmsdmwmbot commented Apr 4, 2023

cmsdmwmbot commented Apr 4, 2023

cmsdmwmbot commented Apr 4, 2023

amaltaro left a comment

Choose a reason for hiding this comment

vkuznet commented Apr 5, 2023

todor-ivanov left a comment

Choose a reason for hiding this comment

vkuznet commented Apr 10, 2023

amaltaro commented Apr 11, 2023

vkuznet commented Apr 12, 2023

amaltaro commented Apr 12, 2023

vkuznet commented Apr 13, 2023

amaltaro left a comment

Choose a reason for hiding this comment