bazelbuild / rules_python

Bazel Python Rules
https://rules-python.readthedocs.io
Apache License 2.0
510 stars 517 forks source link

Issues with AWS CDK >= 2.51.0 #914

Open psalvaggio-dl opened 1 year ago

psalvaggio-dl commented 1 year ago

🐞 bug report

Affected Rule

requirement pip_parse

Is this a regression?

No

Description

With AWS CDK 2.51.0, they have split out a number of sub-packages. Not sure why as they are required dependencies, but nonetheless, if you depend on aws-cdk-lib, you also now depend on:

aws-cdk.asset-awscli-v1
aws-cdk.asset-kubectl-v20
aws-cdk.asset-node-proxy-agent-v5

This seems to break tests in my monorepo. I am not sure exactly why, but my theory is that there are now multiple aws-cdk/__init__.py's in my PYTHONPATH due to rules_python's PYTHONPATH-based solution for handling pip dependencies.

🔬 Minimal Reproduction

I have made a minimal example here: https://github.com/psalvaggio-dl/bazel-cdk-repro

In order to reproduce, the bazel test //py:test can be run.

🔥 Exception or Error

My pytest test prints out the following error:


ImportError while importing test module '/home/ubuntu/.cache/bazel/_bazel_ubuntu/f5bb56bb935f39261247d94ebd292fdf/sandbox/linux-sandbox/5/execroot/bazel-cdk-repro/bazel-out/k8-fastbuild/bin/py/test.runfiles/bazel-cdk-repro/py/test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
py/test.py:1: in 
    from aws_cdk import Aws
E   ImportError: cannot import name 'Aws' from 'aws_cdk' (/home/ubuntu/.cache/bazel/_bazel_ubuntu/f5bb56bb935f39261247d94ebd292fdf/sandbox/linux-sandbox/5/execroot/bazel-cdk-repro/bazel-out/k8-fastbuild/bin/py/test.runfiles/pip_aws_cdk_asset_awscli_v1/site-packages/aws_cdk/__init__.py)
=========================== short test summary I

Aws we can see in the error, it seems to thing that the aws_cdk/__init__.py in pip_aws_cdk_asset_awscli_v1 is the right one, which is why I feel like having multiples in the PYTHONPATH is the issue.

🌍 Your Environment

Operating System:

Ubuntu 22.04

Output of bazel version:

  
bazel 5.3.2
  

Rules_python version:

  
0.15.1
  

Anything else relevant?

philsc commented 1 year ago

You can fix that particular error you posted by doing this:

diff --git a/WORKSPACE.bazel b/WORKSPACE.bazel
index b3c4386..4a986e1 100644
--- a/WORKSPACE.bazel
+++ b/WORKSPACE.bazel
@@ -43,6 +43,7 @@ pip_parse(
     name = "pip",
     requirements_lock = "//py:requirements.txt.lock",
     python_interpreter_target = py_interpreter,
+    enable_implicit_namespace_pkgs = True,
 )

 load("@pip//:requirements.bzl", lazy_pip_install = "install_deps")

That, together with --incompatible_default_to_explicit_init_py gets you past that error.

Unfortunately, you then run into this error:

ImportError while importing test module '/home/phil/.cache/bazel/_bazel_phil/03812bb59b76e0d44a0c22cc9583226f/sandbox/linux-sandbox/17/execroot/bazel-cdk-repro/bazel-out/k8-fastbuild/bin/py/test.runfiles/bazel-cdk-repro/py/test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
py/test.py:1: in <module>
    from aws_cdk import Aws
../pip_aws_cdk_lib/site-packages/aws_cdk/__init__.py:1257: in <module>
    from ._jsii import *
../pip_aws_cdk_lib/site-packages/aws_cdk/_jsii/__init__.py:13: in <module>
    import aws_cdk.asset_awscli_v1._jsii
E   ModuleNotFoundError: No module named 'aws_cdk.asset_awscli_v1'

Digging into it, it seems to be happening because the aws-cdk.lib package contains an __init__.py file in that namespace:

bazel-out/k8-fastbuild/bin/py/test.runfiles/bazel-cdk-repro/external/pip_aws_cdk_lib/site-packages/aws_cdk/__init__.py

I.e. it never finds the following file(s) because Python stopped looking for namespace packages named aws_cdk:

bazel-out/k8-fastbuild/bin/py/test.runfiles/bazel-cdk-repro/external/pip_aws_cdk_asset_awscli_v1/site-packages/aws_cdk/asset_awscli_v1

That means aws-cdk.lib doesn't support namespace packages even though it's distributing its code as such. I would be tempted to call this a bug with aws-cdk.lib, not with rules_python.

psalvaggio-dl commented 1 year ago

Thanks for looking into it. Yes, I can verify that behavior and have updated the reproduction case. I also have an issue open in the CDK https://github.com/aws/jsii/issues/3881, so I will post this over there as well to see if there is something they can fix on their end.

psalvaggio-dl commented 1 year ago

I got some feedback on the Bazel slack and was able to push a fix up to the aspect_rules branch of the repo. It required using the aspect_rules_py for depending on wheels of the AWS CDK directly. This worked in both the small reproduction case as well as my bigger monorepo. This solution is fine with me, but do we want to leave this issue open to see if there might be a way to fix this solely with rules_python?

psalvaggio-dl commented 1 year ago

@philsc I reposted you comment over on the bug in the CDK and they are asking for a bit of clarification on why you think their library doesn't support namespace packages? Could you elaborate on this a bit so I can get direct them on what might need to change in their library to make this work without too much workaround?

philsc commented 1 year ago

I replied there directly :+1:

github-actions[bot] commented 1 year ago

This issue 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!

psalvaggio-dl commented 1 year ago

Commenting since the issue got marked stale. This is still blocking me upgrading this AWS dependency, however, it sounds like this is a bug on their side and a fix on rules_python's side would introduce non-standard compliant behavior that may lead to other issues.

aignas commented 7 months ago

With #1393 you should be able to apply patches to wheels, although it only supports bzlmod. Would that be something that is good enough for your use case?

aignas commented 6 months ago

FYI, for legacy WORKSPACE users it is technically possible to pass whl_patches argument to install_deps() call in your WORKSPACE. The API is not something we want to make a promise to maintain backwards compatibility for, hence the documentation that it is internal use only. As we evolve the APIs to better support patching and cross-platform dependency resolution, this may change, so please be warned, but at least this can give a stop-gap solution without requiring users to patch rules_python itself.

The source code documenting on what you would need to pass is https://github.com/bazelbuild/rules_python/blob/main/python/pip_install/pip_repository.bzl#L847.

AndrewGuenther commented 6 months ago

@aignas A wheel patch won't fix this because the __init__.py at the top level of the aws_cdk_lib package has content in it. It is not a pure namespace package.

github-actions[bot] commented 1 week ago

This issue 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!