-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix detection of epath #26901
Fix detection of epath #26901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Neil!
I replaced |
Unfortunately, the old detection code doesn't guarantee that `epath` is installed: ``` [utM] In [7]: importlib.util.find_spec("etils.epath") Out[7]: ModuleSpec(name='etils.epath', loader=<_frozen_importlib_external.SourceFileLoader object at 0x73b8492a7230>, origin='/home/neil/src/cmm/.venv/lib/python3.12/site-packages/etils/epath/__init__.py', submodule_search_locations=['/home/neil/src/cmm/.venv/lib/python3.12/site-packages/etils/epath']) [utM] In [8]: import etils.epath --------------------------------------------------------------------------- ModuleNotFoundError Traceback (most recent call last) Cell In[8], line 1 ----> 1 import etils.epath ... ModuleNotFoundError: No module named 'importlib_resources' ``` This happened every time I ran jax with a clean environment.
@@ -15,8 +15,8 @@ | |||
from __future__ import annotations | |||
|
|||
import abc | |||
import pathlib | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do want path
instead of pathlib
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want that? Whether you use etils or not, you get a pathlib.Path
(essentially) instance here.
The issue is that jax._src.path.Path
is now Callable[[...], pathlib.Path]
, which can't be used as an annotation.
I had to make it a callable rather than type[pathlib.Path]
because in Python, initializers don't obey LSP, so type[pathlib.Path]
's __init__
is essentially untyped under some type checkers. (I wrote about this, and it appears that MyPy has recently started switching its stance on this based on comments by their team, but I don't have links to that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, ignore me, this is just an interface so pathlib.Path
should be fine. Sorry for the noise and thank you for the additional context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, always a pleasure interacting with the Jax team!
Unfortunately, the old detection code doesn't guarantee that
epath
is installed:This happened every time I ran jax with a clean environment.