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

breaking change in 0.4.0 excluding tests/ folders in wheels #538

Closed
alexeagle opened this issue Sep 23, 2021 · 4 comments · Fixed by #539
Closed

breaking change in 0.4.0 excluding tests/ folders in wheels #538

alexeagle opened this issue Sep 23, 2021 · 4 comments · Fixed by #539

Comments

@alexeagle
Copy link
Collaborator

in 0.4.0 we released this breaking change
7609526

However the tables library imports its own tests 🙄

https://github.com/PyTables/PyTables/blob/v3.6.1/tables/__init__.py#L148

Running bazel test :pandas_test in this repro https://github.com/UebelAndre/rules_python_regression illustrates the problem, and commenting out that line in tables fixes it.

So the question is what to do:

  1. only revert the breaking change
  2. add some opt-in or opt-out behavior all the way up at pip_install or pip_import that is wired into the BUILD file generation, turning this change on for all wheels
  3. make it more fine-grained, such that users configure specific libraries to have the tests folder excluded
  4. generalize the feature like in rules_nodejs where users can give us snippets to replace/extend the content of generated BUILD files for certain wheels. Then when some tests/ files collide with your first-party code you have to give us explicit BUILD file for that library

@mattem this was originally your feature, any opinions?

@UebelAndre
Copy link
Contributor

It'd be great to have a fix then a quick 0.4.1 release after that 🙏

hrfuller added a commit that referenced this issue Sep 28, 2021
…ers (#528)" (#539)

This reverts commit 7609526.

Fixes #538

Co-authored-by: Henry Fuller <hrofuller@gmail.com>
@UebelAndre
Copy link
Contributor

For future readers, the regression test can be found here:
80d72563a83d0a8c7b33b345ac07e608896fdc3fd71ebda4d2ee4a30deafe8bf rules_python_regression-main.zip

rules_python_regression

Run bazel run //:docker

==================== Test output for //:pandas_test:
Traceback (most recent call last):
  File "/root/.cache/bazel/_bazel_root/1dcce0c59a351f514e43f3b471e58117/sandbox/processwrapper-sandbox/2/execroot/pandas_example/bazel-out/k8-fastbuild/bin/pandas_test.runfiles/pandas_example/pandas_test.py", line 5, in <module>
    thing = pandas.HDFStore(os.getenv("HDFS_FILE"), "r")
  File "/root/.cache/bazel/_bazel_root/1dcce0c59a351f514e43f3b471e58117/sandbox/processwrapper-sandbox/2/execroot/pandas_example/bazel-out/k8-fastbuild/bin/pandas_test.runfiles/pip_deps_pypi__pandas/pandas/io/pytables.py", line 572, in __init__
    tables = import_optional_dependency("tables")
  File "/root/.cache/bazel/_bazel_root/1dcce0c59a351f514e43f3b471e58117/sandbox/processwrapper-sandbox/2/execroot/pandas_example/bazel-out/k8-fastbuild/bin/pandas_test.runfiles/pip_deps_pypi__pandas/pandas/compat/_optional.py", line 118, in import_optional_dependency
    raise ImportError(msg) from None
ImportError: Missing optional dependency 'tables'.  Use pip or conda to install tables.
================================================================================
INFO: Elapsed time: 17.878s, Critical Path: 3.39s
INFO: 16 processes: 12 internal, 4 processwrapper-sandbox.
INFO: Build completed, 1 test FAILED, 16 total actions
//:requirements_test                                                     PASSED in 3.0s
//:pandas_test                                                           FAILED in 1.7s
  /root/.cache/bazel/_bazel_root/1dcce0c59a351f514e43f3b471e58117/execroot/pandas_example/bazel-out/k8-fastbuild/testlogs/pandas_test/test.log

INFO: Build completed, 1 test FAILED, 16 total actions

@Panfilwk
Copy link

Panfilwk commented Oct 25, 2021

any update on this getting put out in a release? My team wants to move to 0.4.0 to get this fix to pip_parse, but we're blocked on upgrading by this regression

@alexeagle
Copy link
Collaborator Author

I just released 0.5.0, does that work for you? I can go back and make a release branch for 0.4.1 but it's time that could otherwise be spent fixing rules_nodejs :)

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 a pull request may close this issue.

3 participants