add imports to fix bazel --noexperimental_python_import_all_repositories flag

[AugustKarlstedt]

see bazelbuild/rules_python#792

[aiuto]

It's going to be a week or two before I can look at any rules_pkg things.
I don't doubt that this fixes some existing problem, but it looks like a hack around brokenness in rules_python which should be fixed upstream.

[fmeum]

Just FYI: From what I have heard, it is very likely that this flag will never be flipped. It also breaks with Bzlmod, since repository names are no longer stable identifiers in that world.

[AugustKarlstedt]

Just FYI: From what I have heard, it is very likely that this flag will never be flipped. It also breaks with Bzlmod, since repository names are no longer stable identifiers in that world.

Wdym the flag will never be flipped?

Also, from the rules_python discussion

The basic implication here is that Python libraries that don't include their repo name in their import statements will break. Python libraries that do include their repo name in their import statements will be OK.

the solution in this PR doesn't use the repo name and instead uses a relative import. so from what I understand this does not depend on the repository name. Rather it just ensures that the root path of whatever py_library being used is correctly added to the path, even if bzlmod or other tooling puts it into a different directory.

[fmeum]

Sorry, I should have explained my point better.

Essentially all information I wanted to add is that with Bzlmod, which is planned to become a full replacement for WORKSPACE over the course of two or three Bazel major releases, repository names in runfiles will become implementation details.

As a result, it may no longer be possible for a Python package to import one from a different repo using an import qualified with the repository name. Since setting the flag to false means that that's the only way to import across repos, the flag may no longer be usable by then.

(I'm not much of a Python user myself, I just got in touch with this flag while working on improving the runfiles situation for Bzlmod. It's entirely possible I'm missing the point of this PR)

[aiuto]

I want to see a resolution in bazelbuild/bazel#2636 before trying to work around this. This is something that should be fixed in rules_python and not in every use case.

[AugustKarlstedt]

I don't think this changes any existing behavior so it feels pretty safe imo

rules_docker recently merged the same fix bazelbuild/rules_docker#2171

[AugustKarlstedt]

additionally this flag enforces #412

[amir-f]

Any updates on this PR ?

As a result, it may no longer be possible for a Python package to import one from a different repo using an import qualified with the repository name. Since setting the flag to false means that that's the only way to import across repos, the flag may no longer be usable by then.

In our use case we don't need to import across repos. In our monorepo, for certain py_binary targets that have many third party dependencies PYTHONPATH has ballooned in size to the point that it exceeds the limit set by the OS (see this issue) So we have no choice but to set --experimental_python_import_all_repositories=false.

This PR simply makes py_binary and py_library targets in rules_pkg work when this flag is set to false.

I would love a larger change in bazel and rules_python that would obviate the need for this workaround but I don't see it being planned anywhere AFAIK. In the meantime this workaround seems to be the way to go (see bazelbuild/rules_python#1243 and bazelbuild/rules_docker#2171)

[aiuto]

But won't solutions like this bloat your PYTHONPATH from the import attributes on each library?
ISTM that relative imports would be the only solution which avoids the long PYTHONPATH problem.

[fmeum]

Also worth keeping in mind that --experimental_python_import_all_repositories=false won't work with Bzlmod.

[aiuto]

ISTM that this PR would work in either mode. I just wonder if it actually helps.

[AugustKarlstedt]

@amir-f is exactly right – this is necessary for py_binary targets with many dependencies. I've been using a fork with this change for the past 8 months in our monorepo with no issues. It doesn't affect existing behavior or bzlmod. I feel it should be merged as rules_docker did as it's only beneficial to end users

[fmeum]

Yes, please do, I didn't intend to say anything against merging this. I just wanted to clarify that this may not be a long term solution for the underlying problem.

[aiuto]

Can you merge in from head and push this again. The tests are failing as if we were picking up the WORKSPACE file without the update to a newwe bazelbuild/pllatforms.

[AugustKarlstedt]

Can you merge in from head and push this again. The tests are failing as if we were picking up the WORKSPACE file without the update to a newwe bazelbuild/pllatforms.

will do!

[amir-f]

But won't solutions like this bloat your PYTHONPATH from the import attributes on each library? ISTM that relative imports would be the only solution which avoids the long PYTHONPATH problem.

In our case, the root of monorepo is added to the PYTHONPATH and we don't need to add import = field to individual bazel targets. However with --experimental_python_import_all_repositories=false third party bazel repos (eg. rules_foo) won't have their root added anymore hence the need for explicit import = here.

I understand this is just a work-around.

[AugustKarlstedt]

OK updated with latest main. Looks like tests are passing now.

[aiuto]

Sorry for the slow merge. I was OOO on vacation the last 2 weeks.