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

Implement PEP 562 for Python >= 3.7 #269

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mondeja
Copy link

@mondeja mondeja commented Dec 17, 2022

Currently when importing whatever object from diskcache, if django is installed it is imported. You can see it in the next image generated by pyinstrument:

image

To avoid that, this PR implements PEP 562 on the __init__.py file for Python 3.7 onwards. After it, when importing whatever object, like with from diskcache import Cache for example, django is not imported:

image

According to my benchmarks, this avoids ~100ms of initialization time on all imports except DjangoCache on Python3.8. Especially useful in CLI programs that don't need django and should start as fast as possible.

@mondeja
Copy link
Author

mondeja commented Dec 19, 2022

Current workaround:

import os
import importlib.util

_diskcache_init_path = importlib.util.find_spec("diskcache").origin
_diskcache_core_spec = importlib.util.spec_from_file_location(
    "diskcache.core",
    os.path.join(
        os.path.dirname(_diskcache_init_path),
        "core.py",
    )
)
_diskcache_core = importlib.util.module_from_spec(
    _diskcache_core_spec,
)
_diskcache_core_spec.loader.exec_module(_diskcache_core)

Cache = _diskcache_core.Cache

@grantjenks
Copy link
Owner

Is there any way to refactor this code to a library that implements PEP 562 and could be used instead? As it it is currently, it's helpful but kinda inscrutable.

@mondeja
Copy link
Author

mondeja commented Feb 27, 2024

To be clear, you're asking about a library that supports PEP562 with these kind of exceptions like the djangocache module?

Exists pep562 but unfortunately does not support optional imports like the django one that this library uses, so in the end you still need to create the __getattr__ and __dir__ functions. I don't think it's worth adding a dependency plus all the performance impact of wrapping the module in a class for that.

Perhaps the most comfortable solution would be to force users to import the Django cache from their own module, which would be a breaking change:

from diskcache.djangocache import DjangoCache

Another solution could be to stop supporting Python3.7, which has reached his EOL, so the code could be much more simplified.

Copy link

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you could do:

def __getattr__(attr):
    if attr == "DjangoCache":
        from .djangocache import DjangoCache
        obj = DjangoCache
    else:
        raise AttributeError(f"module {__name__!r} has no attribute {attr!r}")
    globals()[attr] = obj
    return obj

and it would be simpler + allow IDEs and type checkers to still resolve symbols

(Separate module seems like a good alternative idea, at cost of breaking change)

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.

None yet

3 participants