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

Replace pkg_resources usage with packaging + importlib.metadata #392

Merged
merged 5 commits into from
Jul 28, 2022

Conversation

jacob-indigo
Copy link
Contributor

@jacob-indigo jacob-indigo commented Jul 27, 2022

geoalchemy2 is currently fairly slow to import (with and without __pycache__ being built) and, more importantly, it seems gets substantially slower as the Python environment gets larger. This is not an issue with geoalchemy2 itself though. SQLAlchemy is the main culprit for the slow baseline import time and pkg_resources is the culprit for the environment size-dependent import time (see: pypa/setuptools#926).

The PR does two things:

  1. Replaces the use of pkg_resources in shape.py with packaging (an existing dependency)
  2. Attempts to use importlib.metadata instead of pkg_resources in __init__.py, falling back to pkg_resources if you're using Python <3.8

Profiling import time before and after these changes:

Tiny (43M):
    Before: 0.332
    After: 0.252

Medium (383M):
    Before: 0.419
    After: 0.319

Large (478M):
    Before: 0.429
    After: 0.308

X-Large (600M):
    Before: 0.462
    After: 0.316 (1.5x speedup)

The profiling was done in 4 different virtualenvs, covering a range of environment sizes (arbitrarily named here). Tiny is just what's in requirement-rtd.txt plus shapely: requirements.tiny.txt, and the other environments are real example requirements files from internal projects (not included here but available upon request).

These times were produced simply using:

time python -c "import geoalchemy2; import geoalchemy2.shape;"

Additionally, here are some visualizations of the python -X importtime outputs for the same profiling command in the X-Large environment:
Before:
Screen Shot 2022-07-27 at 11 50 16 AM

After:
Screen Shot 2022-07-27 at 11 50 06 AM

@adrien-berchet
Copy link
Member

Hi @jacob-indigo
Thank you very much for this work! I didn't know this issue, that's interesting.

For the implementation details, I think we can group everything into one try block, something like:

try:
    import importlib.metadata
    __version__ = importlib.metadata.version('GeoAlchemy2')
except ImportError:
    try:
        from pkg_resources import DistributionNotFound
        from pkg_resources import get_distribution
        __version__ = get_distribution('GeoAlchemy2').version
    except:  # pragma: no cover
        pass

WDYT?

@jacob-indigo
Copy link
Contributor Author

@adrien-berchet That works for me! Just pushed that change.

@adrien-berchet
Copy link
Member

After some more thinking, I think we have to write this if we want to handle all cases without a bare Exception:

try:
    import importlib.metadata
except ImportError:
    try:
        from pkg_resources import DistributionNotFound
        from pkg_resources import get_distribution
    except ImportError:  # pragma: no cover
        pass
    else:
        try:
            __version__ = get_distribution('GeoAlchemy2').version
        except DistributionNotFound:  # pragma: no cover
            pass
else:
    try:
        __version__ = importlib.metadata.version('GeoAlchemy2')
    except importlib.metadata.PackageNotFoundError:  # pragma: no cover
        pass

Do you agree? Did I miss anything again?

@adrien-berchet
Copy link
Member

LGTM now.
Is it fine for you @jacob-indigo ?

@jacob-indigo jacob-indigo force-pushed the replace_pkg_resources branch from 040777e to c7351b2 Compare July 28, 2022 11:22
@jacob-indigo
Copy link
Contributor Author

@adrien-berchet Good catch on the missing PackageNotFoundError except. lgtm ✅

@adrien-berchet adrien-berchet merged commit ca3cf55 into geoalchemy:master Jul 28, 2022
@adrien-berchet
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants