Fix discoverability of tests for ARM in Breeze #28432

potiuk · 2022-12-17T20:21:30Z

Breeze in case of ARM processor lacks support for several components (because they do not have supported ARM binaries available):

MySQL
MSSQL
LevelDB
Azure Service Bus

When you try to attempt to run pytest on a group of tests that import one of those, the collection failed and none of the tests could run even if some of them could.

This change uses pytest's skip on a module level and local imports in case the tests are inter-mixed with other tests in the same module to avoid import errors during collection.

The try/except pattern over pytest.importorskip is preferred because we are using try/except in a number of other cases and we are pretty familiar with similar pattern and importorskipi has a bit unexpected behaviour (it returns imported module and you do not see the usual import nnnn. Also in our case we often wrap more than one import in one try/except (and it would lead to a duplicating messages to print really.

We also add a separate command in ci to just perform a collection of tests and see if all tests are collectable after uninstalling all those libraries. This would prevent the problems from reapparing.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

eladkal · 2022-12-17T20:26:40Z

Question: will beeze let ARM users know tests are skipped locally but will run in CI?

potiuk · 2022-12-17T20:53:02Z

Question: will beeze let ARM users know tests are skipped locally but will run in CI?

They will not even be able to see them running. Those tests are not runnable on ARM anyway - you cannot run tests with --backend mysql - you will see error that "MySQL is not supported for ARM"

For other tests they will be skipped - and while you will not see that in super "prominent" way (You will see that test was skipped and why".

potiuk · 2022-12-17T20:54:51Z

Oh. NO... again the stupid "docs" treated sometimes as "internall" and sometimes as internal package... I will add isort:skip to get rid of that !

potiuk · 2022-12-17T20:59:50Z

AAAAH. Isort introducing change in sorting order TODAY #!#$@!#%!$%$!@$

Taragolis

+1

Just add information about LevelDB and Azure Service Bus, which could be a follow up and discussion, but not a show stopper for this PR

Taragolis · 2022-12-18T12:36:22Z

tests/system/providers/google/leveldb/example_leveldb.py

Actually we could install selected packages into ARM CI image libleveldb1d, libleveldb-dev and after that we might successfully install plyvel. At least it work when I migrate this test to pytest.

root@e3f4bad5e095:/opt/airflow# apt-get install libleveldb1d libleveldb-dev -y Reading package lists... Done Building dependency tree... Done Reading state information... Done The following additional packages will be installed: libsnappy1v5 Suggested packages: leveldb-doc The following NEW packages will be installed: libleveldb-dev libleveldb1d libsnappy1v5 0 upgraded, 3 newly installed, 0 to remove and 5 not upgraded. Need to get 341 kB of archives. After this operation, 1521 kB of additional disk space will be used. Get:1 http://deb.debian.org/debian bullseye/main arm64 libsnappy1v5 arm64 1.1.8-1 [17.2 kB] Get:2 http://deb.debian.org/debian bullseye/main arm64 libleveldb1d arm64 1.22-3 [132 kB] Get:3 http://deb.debian.org/debian bullseye/main arm64 libleveldb-dev arm64 1.22-3 [192 kB] Fetched 341 kB in 0s (1114 kB/s) Selecting previously unselected package libsnappy1v5:arm64. (Reading database ... 30889 files and directories currently installed.) Preparing to unpack .../libsnappy1v5_1.1.8-1_arm64.deb ... Unpacking libsnappy1v5:arm64 (1.1.8-1) ... Selecting previously unselected package libleveldb1d:arm64. Preparing to unpack .../libleveldb1d_1.22-3_arm64.deb ... Unpacking libleveldb1d:arm64 (1.22-3) ... Selecting previously unselected package libleveldb-dev:arm64. Preparing to unpack .../libleveldb-dev_1.22-3_arm64.deb ... Unpacking libleveldb-dev:arm64 (1.22-3) ... Setting up libsnappy1v5:arm64 (1.1.8-1) ... Setting up libleveldb1d:arm64 (1.22-3) ... Setting up libleveldb-dev:arm64 (1.22-3) ... Processing triggers for libc-bin (2.31-13+deb11u5) ... root@e3f4bad5e095:/opt/airflow# pip install plyvel Collecting plyvel Downloading plyvel-1.5.0.tar.gz (152 kB) ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 152.3/152.3 kB 1.7 MB/s eta 0:00:00 Preparing metadata (setup.py) ... done Building wheels for collected packages: plyvel Building wheel for plyvel (setup.py) ... done Created wheel for plyvel: filename=plyvel-1.5.0-cp37-cp37m-linux_aarch64.whl size=92562 sha256=9ff2a2511ddd6208c681b4f1da198fe6594d57648b5cd87d7d3019c45c22ffc9 Stored in directory: /tmp/pip-ephem-wheel-cache-57dgr3zr/wheels/d7/4e/63/7cd883b7e898b245fd60b5db4fbe805fe0c197e24024aa8c97 Successfully built plyvel Installing collected packages: plyvel Successfully installed plyvel-1.5.0

However we have in setup.py strict limitation about leveldb extra:

airflow/setup.py

Line 307 in 8e0df88

leveldb = ['plyvel; platform_machine != "aarch64"']

Which is inconsistent with provider extra:

airflow/airflow/providers/google/provider.yaml

Lines 1065 to 1067 in 8e0df88

- name: leveldb

dependencies:

- plyvel

Yeah we can attempt to do it as next step. The difference is that the first one was needed for image building that's why aarch exclusion - the second is just extra specification for google provider (and those two might not be synchronized indeed). But if we can build it in the way you described, that would be better than excluding it. I will try it in a moment.

Taragolis · 2022-12-18T12:47:27Z

tests/providers/microsoft/azure/hooks/test_asb.py

We have everything required for install uamqp (requirements for azure-servicebus) into ARM CI image

root@e3f4bad5e095:/opt/airflow# pip install azure-servicebus Collecting azure-servicebus Downloading azure_servicebus-7.8.1-py3-none-any.whl (210 kB) ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 210.4/210.4 kB 2.5 MB/s eta 0:00:00 Requirement already satisfied: isodate>=0.6.0 in /usr/local/lib/python3.7/site-packages (from azure-servicebus) (0.6.1) Requirement already satisfied: azure-core<2.0.0,>=1.14.0 in /usr/local/lib/python3.7/site-packages (from azure-servicebus) (1.26.1) Requirement already satisfied: typing-extensions>=3.7.4.3 in /usr/local/lib/python3.7/site-packages (from azure-servicebus) (4.4.0) Requirement already satisfied: azure-common~=1.1 in /usr/local/lib/python3.7/site-packages (from azure-servicebus) (1.1.28) Collecting uamqp<2.0.0,>=1.5.1 Downloading uamqp-1.6.3.tar.gz (4.4 MB) ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 4.4/4.4 MB 3.9 MB/s eta 0:00:00 Installing build dependencies ... done Getting requirements to build wheel ... done Preparing metadata (pyproject.toml) ... done Requirement already satisfied: six>=1.11.0 in /usr/local/lib/python3.7/site-packages (from azure-servicebus) (1.16.0) Requirement already satisfied: msrest<2.0.0,>=0.6.17 in /usr/local/lib/python3.7/site-packages (from azure-servicebus) (0.7.1) Requirement already satisfied: requests>=2.18.4 in /usr/local/lib/python3.7/site-packages (from azure-core<2.0.0,>=1.14.0->azure-servicebus) (2.28.1) Requirement already satisfied: certifi>=2017.4.17 in /usr/local/lib/python3.7/site-packages (from msrest<2.0.0,>=0.6.17->azure-servicebus) (2022.12.7) Requirement already satisfied: requests-oauthlib>=0.5.0 in /usr/local/lib/python3.7/site-packages (from msrest<2.0.0,>=0.6.17->azure-servicebus) (1.3.1) Requirement already satisfied: idna<4,>=2.5 in /usr/local/lib/python3.7/site-packages (from requests>=2.18.4->azure-core<2.0.0,>=1.14.0->azure-servicebus) (3.4) Requirement already satisfied: urllib3<1.27,>=1.21.1 in /usr/local/lib/python3.7/site-packages (from requests>=2.18.4->azure-core<2.0.0,>=1.14.0->azure-servicebus) (1.26.13) Requirement already satisfied: charset-normalizer<3,>=2 in /usr/local/lib/python3.7/site-packages (from requests>=2.18.4->azure-core<2.0.0,>=1.14.0->azure-servicebus) (2.1.1) Requirement already satisfied: oauthlib>=3.0.0 in /usr/local/lib/python3.7/site-packages (from requests-oauthlib>=0.5.0->msrest<2.0.0,>=0.6.17->azure-servicebus) (3.2.2) Building wheels for collected packages: uamqp Building wheel for uamqp (pyproject.toml) ... done Created wheel for uamqp: filename=uamqp-1.6.3-cp37-cp37m-linux_aarch64.whl size=973019 sha256=447875aa014f66c70afb9e4947df7abee8a96d0ffe35fb915fbeff1ad082b09e Stored in directory: /tmp/pip-ephem-wheel-cache-p3l6jc6o/wheels/29/4e/6c/a5202f34bf3871c1401f0e549dc4e275cdd80bd9dc2977bc0e Successfully built uamqp Installing collected packages: uamqp, azure-servicebus Successfully installed azure-servicebus-7.8.1 uamqp-1.6.3

Unfortunetly azure-servicebus strictly prohibited for ARM in provider requirements

airflow/airflow/providers/microsoft/azure/provider.yaml

Line 67 in 8e0df88

- azure-servicebus>=7.6.1; platform_machine != "aarch64"

Ah. Maybe it has been fixed already. Let me try it as well. Good points :)

Actually this is a "grey zone": uamqp still do not have wheel packages for aarch64 - open issue in Github: Azure/azure-uamqp-python#349, so end users need to pre-install some dev packages for install it from tar.gz distribution,

So if we change requirements for package then someone who intend to use airflow on ARM (yeah this is experimental) faced with issues to install entire provider package.

Breeze in case of ARM processor lacks support for several components (because they do not have supported ARM binaries available): * MySQL * MSSQL * LevelDB * Azure Service Bus When you try to attempt to run pytest on a group of tests that import one of those, the collection failed and none of the tests could run even if some of them could. This change uses pytest's skip on a module level and local imports in case the tests are inter-mixed with other tests in the same module to avoid import errors during collection. The try/except pattern over pytest.importorskip is preferred because we are using try/except in a number of other cases and we are pretty familiar with similar pattern and importorskipi has a bit unexpected behaviour (it returns imported module and you do not see the usual `import nnnn`. Also in our case we often wrap more than one import in one try/except (and it would lead to a duplicating messages to print really. We also add a separate command in ci to just perform a collection of tests and see if all tests are collectable after uninstalling all those libraries. This would prevent the problems from reapparing. Isort fixes are implemented for recently relesed isort version

potiuk · 2022-12-18T17:01:37Z

Actually this is a "grey zone": uamqp still do not have wheel packages for aarch64 - open issue in Github: Azure/azure-uamqp-python#349, so end users need to pre-install some dev packages for install it from tar.gz distribution,

Yep. I just read the discusion in the linked issue in the code. I think I will wait with that for the official wheel. This is not blocking for anyone. If they will build the package, the azure-service-bus willl start working for them - so this is just a matter of automatically pulling it as dependency.

potiuk · 2022-12-18T17:03:41Z

All looks good. Merging.

Breeze in case of ARM processor lacks support for several components (because they do not have supported ARM binaries available): * MySQL * MSSQL * LevelDB * Azure Service Bus When you try to attempt to run pytest on a group of tests that import one of those, the collection failed and none of the tests could run even if some of them could. This change uses pytest's skip on a module level and local imports in case the tests are inter-mixed with other tests in the same module to avoid import errors during collection. The try/except pattern over pytest.importorskip is preferred because we are using try/except in a number of other cases and we are pretty familiar with similar pattern and importorskipi has a bit unexpected behaviour (it returns imported module and you do not see the usual `import nnnn`. Also in our case we often wrap more than one import in one try/except (and it would lead to a duplicating messages to print really. We also add a separate command in ci to just perform a collection of tests and see if all tests are collectable after uninstalling all those libraries. This would prevent the problems from reapparing. Isort fixes are implemented for recently relesed isort version

Breeze in case of ARM processor lacks support for several components (because they do not have supported ARM binaries available): * MySQL * MSSQL * LevelDB * Azure Service Bus When you try to attempt to run pytest on a group of tests that import one of those, the collection failed and none of the tests could run even if some of them could. This change uses pytest's skip on a module level and local imports in case the tests are inter-mixed with other tests in the same module to avoid import errors during collection. The try/except pattern over pytest.importorskip is preferred because we are using try/except in a number of other cases and we are pretty familiar with similar pattern and importorskipi has a bit unexpected behaviour (it returns imported module and you do not see the usual `import nnnn`. Also in our case we often wrap more than one import in one try/except (and it would lead to a duplicating messages to print really. We also add a separate command in ci to just perform a collection of tests and see if all tests are collectable after uninstalling all those libraries. This would prevent the problems from reapparing. Isort fixes are implemented for recently relesed isort version (cherry picked from commit 2a78f50)

potiuk requested review from ashb, kaxil and mik-laj as code owners December 17, 2022 20:21

boring-cyborg bot added area:core-operators area:dev-tools area:providers area:system-tests provider:microsoft-azure Azure-related issues provider:google Google (including GCP) related issues labels Dec 17, 2022

potiuk requested review from Taragolis and eladkal December 17, 2022 20:21

eladkal approved these changes Dec 17, 2022

View reviewed changes

potiuk force-pushed the allow-to-run-all-tests-in-arm branch from dd6a357 to 6617a48 Compare December 17, 2022 21:39

Taragolis approved these changes Dec 18, 2022

View reviewed changes

potiuk force-pushed the allow-to-run-all-tests-in-arm branch from 6617a48 to 1671b4a Compare December 18, 2022 16:19

potiuk merged commit 2a78f50 into apache:main Dec 18, 2022

potiuk deleted the allow-to-run-all-tests-in-arm branch December 18, 2022 17:03

potiuk mentioned this pull request Dec 18, 2022

Re-enable azure service bus on ARM as it now builds cleanly #28442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix discoverability of tests for ARM in Breeze #28432

Fix discoverability of tests for ARM in Breeze #28432

Uh oh!

potiuk commented Dec 17, 2022

Uh oh!

eladkal commented Dec 17, 2022

Uh oh!

potiuk commented Dec 17, 2022

Uh oh!

potiuk commented Dec 17, 2022

Uh oh!

potiuk commented Dec 17, 2022

Uh oh!

Taragolis left a comment

Uh oh!

Taragolis Dec 18, 2022

Uh oh!

potiuk Dec 18, 2022

Uh oh!

Taragolis Dec 18, 2022

Uh oh!

potiuk Dec 18, 2022

Uh oh!

Taragolis Dec 18, 2022

Uh oh!

potiuk commented Dec 18, 2022

Uh oh!

potiuk commented Dec 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix discoverability of tests for ARM in Breeze #28432

Fix discoverability of tests for ARM in Breeze #28432

Uh oh!

Conversation

potiuk commented Dec 17, 2022

Uh oh!

eladkal commented Dec 17, 2022

Uh oh!

potiuk commented Dec 17, 2022

Uh oh!

potiuk commented Dec 17, 2022

Uh oh!

potiuk commented Dec 17, 2022

Uh oh!

Taragolis left a comment

Choose a reason for hiding this comment

Uh oh!

Taragolis Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

potiuk Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

Taragolis Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

potiuk Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

Taragolis Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

potiuk commented Dec 18, 2022

Uh oh!

potiuk commented Dec 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants