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

Fix detection of epath #26901

Merged
merged 1 commit into from
Mar 4, 2025
Merged

Fix detection of epath #26901

merged 1 commit into from
Mar 4, 2025

Conversation

NeilGirdhar
Copy link
Contributor

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.

Copy link
Collaborator

@superbobry superbobry left a comment

Choose a reason for hiding this comment

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

Thanks, Neil!

@google-ml-butler google-ml-butler bot added kokoro:force-run pull ready Ready for copybara import and testing labels Mar 4, 2025
@NeilGirdhar
Copy link
Contributor Author

I replaced epath_installed to fix the build errors, and got rid of the lazy loading since it doesn't do anything anymore. (If the lazy loading is desired, epath may need to be changed so that the old importlib tests work.)

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

Copy link
Collaborator

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.

Copy link
Contributor Author

@NeilGirdhar NeilGirdhar Mar 4, 2025

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.)

Copy link
Collaborator

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.

Copy link
Contributor Author

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!

@copybara-service copybara-service bot merged commit b238bad into jax-ml:main Mar 4, 2025
19 checks passed
@NeilGirdhar NeilGirdhar deleted the etils branch March 4, 2025 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull ready Ready for copybara import and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants