perf: Avoid module-level importlib.util.find_spec

[MarcoGorelli]

What type of PR is this? (check all applicable)

• 💾 Refactor
• ✨ Feature
• 🐛 Bug Fix
• 🔧 Optimization
• 📝 Documentation
• ✅ Test
• 🐳 Other

Related issues

• Related issue #<issue number>
• Closes #<issue number>

Checklist

• Code follows style guide (ruff)
• Tests added
• Documented the changes

If you have comments or can explain your changes, please do so below

[dangotbanned]

@MarcoGorelli could we slow down a bit on this one?

My read on this, was that the import wasn't the issue?

pyinstaller/pyinstaller-hooks-contrib#902 (comment)

Sure, thanks - would it help if we just removed the runtime import from Narwhals anyway?

Sure, if you don't need it. (But dynamic import with find_spec is not really the problematic part - it is the metadata query. Because while our analysis will miss the reference to typing_extensions made via find_spec, there are plenty of direct import typing_extensions later on, which allow us to pick up the module - mostly because we do not handle TYPE_CHECKING as no-op codepath...).

You could also catch the importlib.metadata.PackageNotFoundError when calling importlib.metadata.version(_TYPING_EXTENSIONS) in narwhals-dev/narwhals@7059f5b/narwhals/_translate.py#L20 (blame).

I just wanna be sure I understand the issue fully first

Edit

I replied in (pyinstaller/pyinstaller-hooks-contrib#902 (comment))

[dangotbanned]

@MarcoGorelli okay it seems we're all good! 🎉

pyinstaller/pyinstaller-hooks-contrib#902 (comment)

As far as I'm concerned, there's no need to change anything on your side.

I followed up and checked, the fix they've added is pretty standard and they've used it in lots of popular packages

• https://github.com/search?q=repo%3Apyinstaller/pyinstaller-hooks-contrib%20copy_metadata&type=code

[MarcoGorelli]

sure, thanks, should we just address

You could also catch the importlib.metadata.PackageNotFoundError when calling importlib.metadata.version(_TYPING_EXTENSIONS) in

in case that affects anyone else?

[dangotbanned]

sure, thanks, should we just address

You could also catch the importlib.metadata.PackageNotFoundError when calling importlib.metadata.version(_TYPING_EXTENSIONS) in

in case that affects anyone else?

@MarcoGorelli how would you feel about making that a sub-task of adding a _typing_compat module?

• https://docs.astral.sh/ruff/settings/#lint_typing-modules
• https://mypy.readthedocs.io/en/stable/config_file.html#confval-custom_typing_module
• pyright seems to not need config

We could have that extra safety there, plus:

• A single place to handle TypeVar defaults
• Cut down on the Protocol38 boilerplate that I've littered
• Fun stuff like @deprecated

[MarcoGorelli]

I don't really mind, but based on #2479 I still think we may want to consider removing this - the else branch you've added seems to cover pre-3.13 behaviour perfectly

[dangotbanned]

I don't really mind, but based on #2479 I still think we may want to consider removing this

Thanks @MarcoGorelli!

(#2479) points towards a cost that we can only avoid by not doing the find_spec call.
Since a TypeVar must be defined at the module-level (tried to find a ref but had no luck 😔) the only option we have is to use the else branch.

Appreciate you bearing with me on this 😅

I'd still like to clean up the bits mentioned in (#2391 (comment)) soon - but we might as well get the perf boost in now 🎁

[MarcoGorelli]

sure! thanks a tonne for your attention to detail, here and everywhere