MadAnalysis / madanalysis5

A package for event file analysis and recasting of LHC results
http://madanalysis.irmp.ucl.ac.be
GNU General Public License v3.0
22 stars 18 forks source link

Change in importlib #237

Open jackaraz opened 8 months ago

jackaraz commented 8 months ago

System Settings

for python version > 3.9

Describe the bug

Importlib will crash. The find_spec function moved from the util module see this link.

To Reproduce

run ma5

Expected behaviour

No response

Log files

No response

Additional information

No response

sihyunjeon commented 8 months ago

Hello Jack,

I was just going through madanalysis installation and was going to report this but I found that you already took note of the same issue. it works if lines change like below

from importlib import util

...

# Checking that the 'six' package is present
if not util.find_spec("six"):
jackaraz commented 8 months ago

Hi @sihyunjeon, thanks for the comment! I just need to make sure that it will work in multiple versions of Python at the same time. Probably will need to implement some kind of error handling, will see, thanks!

jackaraz commented 8 months ago

Dev Note: pkg_resources has a get_distribution function, which can be used to check existing packages. If a package is not found, it raises the pkg_resources.DistributionNotFound error. This is more efficient than importing packages. I believe pkg_resources is universal for all Python versions (needs to be checked)

sihyunjeon commented 7 months ago

hi @jackaraz Do you plan to re-release v.1.10.12 or should i wait for v1.10.13 to adapt to this change?

jackaraz commented 7 months ago

Hi @sihyunjeon, we will release a new version, but we are all busy atm, so it might take some time along with other planned releases.

davo9819 commented 2 months ago

There is already a check of python supported versions at the start of bin/ma5

import importlib
import os
import sys

# Checking if the correct release of Python is installed
if sys.version_info[0] != 3 or sys.version_info[1] <= 6:
    sys.exit(
        "Python release "
        + sys.version
        + " is detected.\n"
        + "MadAnalysis 5 works only with Python version 3.6 or more recent version.\n"
        + "Please upgrade your Python installation."
    )

by adding a minor version check the issue can be solved as purposed here

if sys.version_info[1] >= 9:
  import importlib.util

and the code would work as intended.

Actually that would work with any supported version because explicitly importing a submodule exposed in the __init__.py of importlib module is not an overhead, it just looks a bit weird, but a comment explaining the hack should be enough.

In the end it can look like

import importlib.util  # Python >= 3.9 compatibility issue (https://github.com/MadAnalysis/madanalysis5/issues/237) 
import importlib
import os
import sys

# Checking if the correct release of Python is installed
if sys.version_info[0] != 3 or sys.version_info[1] <= 6:
    sys.exit(
        "Python release "
        + sys.version
        + " is detected.\n"
        + "MadAnalysis 5 works only with Python version 3.6 or more recent version.\n"
        + "Please upgrade your Python installation."
    )

I'm happy to makea a PR with the fix.

davo9819 commented 2 months ago

Actually I just tested python 3.6, python 3,7 and python3.8 and all failed with AttributeError: module 'importlib' has no attribute 'util'.

The problem seems to be that some libraries bring importlib.util to scope (like REPL) silently and user code works, but with a clean venv this is not the case.

So an actual fix would be explicitly import importlib.util with every version of python.

jackaraz commented 2 months ago

Thanks, @davo9819; I'll take a look when I have some time!