Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change in importlib #237

Open
jackaraz opened this issue Jan 18, 2024 · 8 comments · May be fixed by #278
Open

Change in importlib #237

jackaraz opened this issue Jan 18, 2024 · 8 comments · May be fixed by #278
Labels
🐛bug Something isn't working

Comments

@jackaraz
Copy link
Member

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

@jackaraz jackaraz added the 🐛bug Something isn't working label Jan 18, 2024
@sihyunjeon
Copy link

sihyunjeon commented Feb 4, 2024

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
Copy link
Member Author

jackaraz commented Feb 5, 2024

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
Copy link
Member Author

jackaraz commented Feb 6, 2024

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
Copy link

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
Copy link
Member Author

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
Copy link

davo9819 commented Jul 20, 2024

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
Copy link

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants