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

feat: Introduce compatibility with native namespace packages #1205

Merged
merged 13 commits into from
Jun 26, 2023

Conversation

jaraco
Copy link
Contributor

@jaraco jaraco commented Dec 20, 2022

This short chain demonstrates a missed expectation, that the google-auth package will mask the presence of other packages in a google namespace when installed. This issue affects corporate packages internally.

The pkg_resources style namespace is the least preferable and least compatible technique indicated in the docs.

By removing the pkg_resources technique, it makes the pkgutil technique the default, adds compatibility with native namespace packages.

After Python 2.7 support is dropped, this package could consider dropping the pkgutil technique as well and instead use native namespace packages, but that's a consideration for a later time.

@jaraco
Copy link
Contributor Author

jaraco commented Dec 20, 2022

I'll address the 'conventionalcommits' errors after there's some consent on the general approach.

@clundin25
Copy link
Contributor

This seems reasonable to me but I think it's best for @parthea to sign off.

@clundin25
Copy link
Contributor

Blocked on b/266148789

@jaraco
Copy link
Contributor Author

jaraco commented Jan 25, 2023

Blocked on b/266148789

Is it blocked? This change is at least theoretically compatible with Python 2. It may have compatibility concerns with other google.* packages.

@clundin25
Copy link
Contributor

@jaraco I spoke with @parthea and ideally we want to do this change across the client libraries in conjunction with dropping Python2.7. Please ping us if we need to adjust the priority of this

@clundin25
Copy link
Contributor

This work is now unblocked and can be picked up. I'll sync up with the team on next steps.

@clundin25
Copy link
Contributor

Since we can now drop support for 2.7, I understand the recommended solution is this one https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages?

@parthea
Copy link
Contributor

parthea commented May 19, 2023

python 2.7 is still supported in setup.py . Let's wait until 2.7 is dropped so we can remove __init__.py and pkgutil altogether.

python_requires=">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*,!=3.5.*",

@jaraco
Copy link
Contributor Author

jaraco commented May 21, 2023

Since we can now drop support for 2.7, I understand the recommended solution is this one https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages?

Yes, that's what I would recommend. But here's the caveat:

If there are other packages installed by the user that implement google.*, those packages may be bringing with them an __init__.py and a different namespace package technique, which could in theory cause these packages not to interoperate. In many cases, they do interoperate due to the way pip omits __init__.py for namespace packages or factors.

Does anyone know if there are other public packages that install behavior into the google.* namespace?

Also, would you like me to amend this PR to utilize the native namespace packages instead?

@clundin25
Copy link
Contributor

I'll start work on removing Python 2.7 support in setup.py.

Is it right that the level of risk between the native namespace and the solution in the PR is the same?

I think this will have to be marked as a breaking change (correct me if I am wrong @parthea)? My preference is to go ahead with the native namespace, assuming the risk is the same.

@jaraco
Copy link
Contributor Author

jaraco commented Jun 5, 2023

I'll start work on removing Python 2.7 support in setup.py.

Is it right that the level of risk between the native namespace and the solution in the PR is the same?

With Python 2.7 support removed, I believe the answer is yes. There are potentially (likely) rare, subtle edge cases, but for the most part, the risks are comparable. Since the pkgutil-style namespaces are older and more mature, they do have better support in older environments, but these concerns are years old and probably ignorable at this point.

I think this will have to be marked as a breaking change (correct me if I am wrong @parthea)? My preference is to go ahead with the native namespace, assuming the risk is the same.

My instinct is to mark it as a breaking change and to indicate that the breaking aspect is ignorable for most users but that the package should not be installed alongside other packages supplying google.* using pkgutil-style namespaces.

I'll update the PR to this new expectation.

@jaraco
Copy link
Contributor Author

jaraco commented Jun 5, 2023

The latest commit proposes instead to use native namespace packages for "google", eliminating the google/__init__.py file and switching to find_namespace_packages to recognize that directory as a package.

I'm not confident that's the correct invocation, because setuptools will pick up any directory as a package (possibly only if it has .py files in it). That means that paths like scripts/ will now be installed as packages where they weren't before.

We'll want to experiment/test to ensure consistency and correctness with this new change.

@jaraco
Copy link
Contributor Author

jaraco commented Jun 5, 2023

The issue can be illustrated thus:

 ~ $ pip-run git+https://github.com/jaraco/google-auth-library-python@bugfix/namespace-support -- -c "import importlib.metadata as md, pprint; pprint.pprint(md.distribution('google-auth').files)"
[PackagePath('docs/__pycache__/conf.cpython-311.pyc'),
 PackagePath('docs/conf.py'),
 PackagePath('google/auth/__init__.py'),
...
 PackagePath('google_auth-2.19.1.dist-info/top_level.txt'),
 PackagePath('samples/cloud-client/snippets/__pycache__/authenticate_explicit_with_adc.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/__pycache__/authenticate_implicit_with_adc.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/__pycache__/idtoken_from_impersonated_credentials.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/__pycache__/idtoken_from_metadata_server.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/__pycache__/idtoken_from_service_account.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/__pycache__/noxfile.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/__pycache__/noxfile_config.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/__pycache__/snippets_test.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/__pycache__/verify_google_idtoken.cpython-311.pyc'),
 PackagePath('samples/cloud-client/snippets/authenticate_explicit_with_adc.py'),
 PackagePath('samples/cloud-client/snippets/authenticate_implicit_with_adc.py'),
 PackagePath('samples/cloud-client/snippets/idtoken_from_impersonated_credentials.py'),
 PackagePath('samples/cloud-client/snippets/idtoken_from_metadata_server.py'),
 PackagePath('samples/cloud-client/snippets/idtoken_from_service_account.py'),
 PackagePath('samples/cloud-client/snippets/noxfile.py'),
 PackagePath('samples/cloud-client/snippets/noxfile_config.py'),
 PackagePath('samples/cloud-client/snippets/snippets_test.py'),
 PackagePath('samples/cloud-client/snippets/verify_google_idtoken.py')]

It's the docs and samples that end up getting pulled into the package inadvertently.

@clundin25
Copy link
Contributor

It's the docs and samples that end up getting pulled into the package inadvertently.

If I understand correctly, I'm okay with pre-emptively excluding the scripts directory, even though it does not have any *.py files.

How can I assist in testing this feature? The PR looks good to me, and the approach seems sound.

@jaraco
Copy link
Contributor Author

jaraco commented Jun 7, 2023

How can I assist in testing this feature?

The change includes a test, so the basic functionality is protected by CI.

Where the biggest uncertainly lies in how this change interacts with other libraries in the google.* namespace. I found, for example, python-cloud-core still uses pkg_resources namespaces. Depending on the installation method, a user installing both could encounter issues (and since google-auth-core depends on google-auth, that's a near certainty).

I did some testing to check and indeed the packages seem to work well together in the same environment when installed by pip:

 draft $ py -m venv .venv
 draft $ py -m pip install -q google-cloud-core google-auth@git+https://github.com/jaraco/google-auth-library-python@bugfix/namespace-support
 draft $ ls .venv/lib/python3.12/site-packages/google/__init__.py
ls: .venv/lib/python3.12/site-packages/google/__init__.py: No such file or directory
 draft $ ls .venv/lib/python3.12/site-packages/google/cloud/__init__.py
ls: .venv/lib/python3.12/site-packages/google/cloud/__init__.py: No such file or directory
 draft $ py -c 'import google.cloud, google.auth' && echo done
done

Even though google.cloud still has the pkg_resources namespace packaging behavior:

 draft $ cat .venv/lib/python3.12/site-packages/google_cloud_core-2.3.2-py3.10-nspkg.pth
import sys, types, os;has_mfs = sys.version_info > (3, 5);p = os.path.join(sys._getframe(1).f_locals['sitedir'], *('google',));importlib = has_mfs and __import__('importlib.util');has_mfs and __import__('importlib.machinery');m = has_mfs and sys.modules.setdefault('google', importlib.util.module_from_spec(importlib.machinery.PathFinder.find_spec('google', [os.path.dirname(p)])));m = m or sys.modules.setdefault('google', types.ModuleType('google'));mp = (m or []) and m.__dict__.setdefault('__path__',[]);(p not in mp) and mp.append(p)
import sys, types, os;has_mfs = sys.version_info > (3, 5);p = os.path.join(sys._getframe(1).f_locals['sitedir'], *('google',));importlib = has_mfs and __import__('importlib.util');has_mfs and __import__('importlib.machinery');m = has_mfs and sys.modules.setdefault('google', importlib.util.module_from_spec(importlib.machinery.PathFinder.find_spec('google', [os.path.dirname(p)])));m = m or sys.modules.setdefault('google', types.ModuleType('google'));mp = (m or []) and m.__dict__.setdefault('__path__',[]);(p not in mp) and mp.append(p)
import sys, types, os;has_mfs = sys.version_info > (3, 5);p = os.path.join(sys._getframe(1).f_locals['sitedir'], *('google', 'cloud'));importlib = has_mfs and __import__('importlib.util');has_mfs and __import__('importlib.machinery');m = has_mfs and sys.modules.setdefault('google.cloud', importlib.util.module_from_spec(importlib.machinery.PathFinder.find_spec('google.cloud', [os.path.dirname(p)])));m = m or sys.modules.setdefault('google.cloud', types.ModuleType('google.cloud'));mp = (m or []) and m.__dict__.setdefault('__path__',[]);(p not in mp) and mp.append(p);m and setattr(sys.modules['google'], 'cloud', m)

But something else I'm noticing now, even the pkg_resources-managed namespace packages, when installed with pip, are showing up as PEP 420 namespace packages (note NamespaceLoader for google and google.cloud):

 draft $ py -q
>>> import google.cloud, google.auth
>>> google.__spec__
ModuleSpec(name='google', loader=<_frozen_importlib_external.NamespaceLoader object at 0x1026f8890>, submodule_search_locations=_NamespacePath(['/Users/jaraco/draft/.venv/lib/python3.12/site-packages/google']))
>>> google.auth.__spec__
ModuleSpec(name='google.auth', loader=<_frozen_importlib_external.SourceFileLoader object at 0x10287f7a0>, origin='/Users/jaraco/draft/.venv/lib/python3.12/site-packages/google/auth/__init__.py', submodule_search_locations=['/Users/jaraco/draft/.venv/lib/python3.12/site-packages/google/auth'])
>>> google.cloud.__spec__
ModuleSpec(name='google.cloud', loader=<_frozen_importlib_external.NamespaceLoader object at 0x1026f8ad0>, submodule_search_locations=_NamespacePath(['/Users/jaraco/draft/.venv/lib/python3.12/site-packages/google/cloud']))

That indicates to me that the compatibility story has improved from years ago when the analysis was done on cross-compatibility.

I do note, however, that other installation methods (older versions of pip or other installers) may produce different results.

Feel free to use these explorations as inspiration for other ways to test the downstream effects of this 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.

4 participants