rules_python icon indicating copy to clipboard operation
rules_python copied to clipboard

fix(bzlmod): make import rules_python.python.runfiles work under bzlmod

Open rickeylev opened this issue 1 year ago • 9 comments

In bzlmod, the name of the @rules_python repo directory name in the runfiles tree changed from simply rules_python to rules_python~VERSION, where "VERSION" includes the module version number and some other parts. This broke import rules_python. Worse, the name isn't knowable in advance and isn't a valid Python package name, so making it importable is very difficult. This makes it harder to switch to using bzlmod.

As a short term solution, we made import python work, which works but (1) requires users to update their code, and (2) requires us to take the more generic "python" top-level name, neither of which are things we want.

To fix, create an importable directory named "rules_python" and set it up so that import rules_python.python.runfiles works.

This importable rules_python directory is created under a src sub-directory so that we can better control our public interface, and, eventually, reduce the number of sys.path entries needed to just that single src directory.

For now, it's kept simple: rules_python just imports pythons.runfiles and patches up sys.modules to make it appear as-if it was an actual sub-module. This is done so that import python.runfiles still works and looks the same (e.g. same __file__ etc).

This unfortunately means an extra entry on sys.path ($repoRoot is added by //python/runfiles:runfiles; $repoRoot/src is added by //src/rules_python:__init__). Fixing that can be done in a future change by moving the files around and doing some different import patching.

Fixes https://github.com/bazelbuild/rules_python/issues/1679

TODO: Update changelog

rickeylev avatar Jan 09 '24 23:01 rickeylev

Sending this out a bit earlier to get feedback. Hence no tests. This was inspired by a comment you (@aignas) made in a a recent PR.

Another approach I considered was __path__.append(dirnames(python.__file__)). I didn't really like that because it meant import rules_python.python; import python; rule_python.python is python was False, i.e. the same code gets imported twice.

I'm not sure how much we should worry about the location of the implementation. I could just move everything under srcs/rules_python, and then setup shims and aliases for import python to work. I think this would be OK? I'm half tempted to just do that in this PR.

rickeylev avatar Jan 09 '24 23:01 rickeylev

I was also looking at namespace packages and if those would work, but I don't think they will or are a good fit. I was looking into this because having rules_python be a namespace package seems like a good idea -- it would give us flexibility to control sub-units of libraries, should we need it.

First, implicit namespace creation would ruin the whole thing, I think. We'll turn that off, eventually, but even if it was off, I think we need a separate sys.path entry for each "distribution" ? This part isn't really clear to me.

rickeylev avatar Jan 09 '24 23:01 rickeylev

Agree with @aignas, I think we need to first decide if we want to ship this as python package (via PyPI) or a bazel package (via bzlmod), then decide if it's confusing or not in terms of how the user would import the library into their code.

groodt avatar Jan 10 '24 20:01 groodt

Out of interest, what are the other language rules doing for Runfiles support libraries these days? I'm wondering what rules_jvm_external rules_go etc are doing.

groodt avatar Jan 10 '24 21:01 groodt

I think we need to first decide if we want to ship this as python package (via PyPI) or a bazel package (via bzlmod), then decide if it's confusing or not in terms of how the user would import the library into their code.

Unfortunately, that ship has pretty much sailed, at least for now. Both styles of dependency are in used today. That import rules_python.python.runfiles works in workspace, but not bzlmod, is essentially a regression (this was reported back as early as April 2023 in the bazel repo).

I do agree we should try to figure out a coherent story here, though.

rickeylev avatar Jan 10 '24 22:01 rickeylev

I guess I think that allowing to consume the runfiles via @rules_python//<some-label> and the PyPI package should be allowed, but in the distant (or maybe not-so) I would love to see the same Python imports working across both. So maybe we could setup the @rules_python//<some-label> import to support both from runfiles import Runfiles and from rules_python.python.runfiles import Runfiles and then nudge people towards using the first import path, as it is the same as the one PyPI provides?

aignas avatar Jan 11 '24 00:01 aignas

same Python imports working across both.

I'm not sure this will be possible. The basic issue is a multi-version conflict: you can get one version of runfiles from pypi and a different version from @rules_python. Users will need some way to resolve that (e.g. being able to force one earlier in the import search), or we need a compelling technical reason why one is strictly better than the other.

This PR has turned into quite a bit of import trickery to make a src-layout work while providing backwards compatibility, so I don't want to introduce a third way of import runfiles and associated trickery.

rickeylev avatar Jan 12 '24 09:01 rickeylev

posterity, from the maintainers meeting:

  • re: consolidating how to import runfiles: we'll just continue on as we are now
  • re: src based layout: It's a bit of a wash because we have to use "src-d" to help avoid conflicts with actual src-based layouts. Moving all the files also has some risk, too. I'll switch this over to adding a top-level rules_python/__init__.py shim that uses __path__ instead and see how that works.

rickeylev avatar Jan 30 '24 03:01 rickeylev

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

github-actions[bot] avatar Aug 13 '24 22:08 github-actions[bot]