Skip to content

ARROW-17389: [Python] Properly exclude tests when PYARROW_INSTALL_TESTS=0 - #13904

Merged
jorisvandenbossche merged 3 commits into
apache:masterfrom
milesgranger:ARROW-17389_rm-tests-from-builds
Aug 23, 2022
Merged

ARROW-17389: [Python] Properly exclude tests when PYARROW_INSTALL_TESTS=0#13904
jorisvandenbossche merged 3 commits into
apache:masterfrom
milesgranger:ARROW-17389_rm-tests-from-builds

Conversation

@milesgranger

@milesgranger milesgranger commented Aug 17, 2022 •
edited by jorisvandenbossche
Loading

Copy link
Copy Markdown
Contributor

Will fix ARROW-17389

It appears when PYARROW_INSTALL_TESTS=0, setuptools would continue to add importable packages anyway:
(would also include warnings for pyarrow.tests and other sub packages.)

    ############################
    # Package would be ignored #
    ############################
    Python recognizes 'pyarrow.tests.parquet' as an importable package,
    but it is not listed in the `packages` configuration of setuptools.

    'pyarrow.tests.parquet' has been automatically added to the distribution only
    because it may contain data files, but this behavior is likely to change
    in future versions of setuptools (and therefore is considered deprecated).

    Please make sure that 'pyarrow.tests.parquet' is included as a package by using
    the `packages` configuration field or the proper discovery methods
    (for example by using `find_namespace_packages(...)`/`find_namespace:`
    instead of `find_packages(...)`/`find:`).

    You can read more about "package discovery" and "data files" on setuptools
    documentation page.

Adding in an explicit package_dir seems to resolve the issue.

Tested locally with both bdist and bdist_wheel
sdist will still include tests, as I believe it's using MANIFEST.in.

$ PYARROW_INSTALL_TESTS=0 python setup.py bdist_wheel > output.txt
$ cat output.txt | grep tests | wc -l
0

@milesgranger milesgranger changed the title ARROW-17389: [Python] Exclude Python conftest and tests when PYARROW_INSTALL_TESTS=0 ARROW-17389: [Python] Exclude conftest and tests when PYARROW_INSTALL_TESTS=0 Aug 17, 2022
@musicinmybrain

musicinmybrain commented Aug 17, 2022

Copy link
Copy Markdown

Testing this on the libarrow package in Fedora Linux, specifically on top of my open PR, I find that it works as advertised with respect to pyarrow.tests, which I’m quite happy about. However, I do find that pyarrow.conftest is still installed.

@github-actions

github-actions Bot commented Aug 17, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-17389

@milesgranger

milesgranger commented Aug 17, 2022

Copy link
Copy Markdown
Contributor Author

I can't seem to convince setuptools to ignore a specific file using exclude. However, can "trick" it by instead having a pyarrow.conftest package with everything currently in conftest.py moved into pyarrow.conftest.__init__. 😅

Probably shouldn't do that though. Would be easier maybe to move tests out of pyarrow but I think there are situations where tests are desired to be packaged up, and perhaps just as tricky to do the inverse in that scenario?

cc @jorisvandenbossche

@jorisvandenbossche

jorisvandenbossche commented Aug 17, 2022

Copy link
Copy Markdown
Member

There are a bunch of failures, because now test data is no longer included by default?

(if we don't find an easy way to exclude conftest.py, I would just leave it like that; it's already useful you discovered that the tests weren't actually excluded)

@milesgranger

milesgranger commented Aug 17, 2022

Copy link
Copy Markdown
Contributor Author

Bare with me, I have it working locally, just need to sort out CI discrepancies. 😅

@milesgranger

milesgranger commented Aug 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Switching in 12c0fff to move pyarrow/tests to tests. Still installing them in the pyarrow.tests when PYARROW_INSTALL_TESTS is set. This creates a clear distinction between test related files and avoids all test related stuff from being included if not desired.

AppVeyor is failing as expected in other PRs and master, and the docs I think is un-releated.

@milesgranger

milesgranger commented Aug 18, 2022

Copy link
Copy Markdown
Contributor Author

Reference to setuptools adding back excluded packages: pypa/setuptools#3340 (comment)

@milesgranger
Exclude Python conftest and tests when PYARROW_INSTALL_TESTS not on
aa05424
@milesgranger
Restructure tests into top level dir
d9d5c71 
This excludes all test related files and data
unless PYARROW_INSTALL_TEST is off. Otherwise
tests is installed as pyarrow.tests as before.
@milesgranger
Use exclude_package_data
d191bd7
@milesgranger

milesgranger commented Aug 18, 2022

Copy link
Copy Markdown
Contributor Author

After a tour of different ways to avoid adding the tests, seems maybe using exclude_package_data is the easiest. Unfortunately it'll still come with pyarrow/conftest.py but even moving tests out of the package directory would still require it due to doc testing needing conftest to run from inside pyarrow.

Leaving the different implementations for any passersby. 😄

@musicinmybrain

musicinmybrain commented Aug 18, 2022

Copy link
Copy Markdown

Thank you for taking the time to investigate this.

@milesgranger

milesgranger commented Aug 23, 2022

Copy link
Copy Markdown
Contributor Author

@jorisvandenbossche do you think this is ready?

jorisvandenbossche
jorisvandenbossche approved these changes Aug 23, 2022
View reviewed changes

@jorisvandenbossche jorisvandenbossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks!

@jorisvandenbossche jorisvandenbossche changed the title ARROW-17389: [Python] Exclude conftest and tests when PYARROW_INSTALL_TESTS=0 ARROW-17389: [Python] Properly exclude tests when PYARROW_INSTALL_TESTS=0 Aug 23, 2022
@jorisvandenbossche
jorisvandenbossche merged commit 8b89147 into apache:master Aug 23, 2022
@jorisvandenbossche

jorisvandenbossche commented Aug 23, 2022

Copy link
Copy Markdown
Member

In the end this didn't "really" close ARROW-17389, since conftest.py is still not excluded. But I am not sure it is worth keeping open that issue?

@milesgranger
milesgranger deleted the ARROW-17389_rm-tests-from-builds branch August 23, 2022 07:30
@milesgranger

milesgranger commented Aug 23, 2022

Copy link
Copy Markdown
Contributor Author

Unfortunate, I agree, at least the bulk of tests are gone.

Even though exclude_package_data considers non-python files, thus why we couldn't easily exclude it there.

AFAIK, if we really want to close it

  • Wait for setuptools update to not auto-include, then convert conftest.py to pyarrow.conftest.__init__.py and exclude pyarrow.conftest
  • Custom setup(cmdclass, ...) to exclude files.
  • ....something else?

@ursabot

ursabot commented Aug 23, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 9882df5 and contender = 8b89147. 8b89147 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.2% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.55% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️1.81% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 8b891471 ec2-t3-xlarge-us-east-2
[Finished] 8b891471 test-mac-arm
[Failed] 8b891471 ursa-i9-9960x
[Finished] 8b891471 ursa-thinkcentre-m75q
[Finished] 9882df58 ec2-t3-xlarge-us-east-2
[Failed] 9882df58 test-mac-arm
[Failed] 9882df58 ursa-i9-9960x
[Finished] 9882df58 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot

ursabot commented Aug 23, 2022

Copy link
Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
@milesgranger @zagto
ARROW-17389: [Python] Properly exclude tests when PYARROW_INSTALL_TES…
18f4782 
…TS=0 (apache#13904)

Authored-by: Miles Granger <[email protected]>
Signed-off-by: Joris Van den Bossche <[email protected]>
scarlettgo pushed a commit to scarlettgo/libarrow that referenced this pull request Apr 6, 2025
@musicinmybrain
Update pyarrow test patch (PR#13904)
b15bcde 
Use a squashed patch corresponding to the final version of
apache/arrow#13904. Fixes FTBFS on F38.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees

No one assigned

Labels

Component: Documentation Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@milesgranger @musicinmybrain @jorisvandenbossche @ursabot