You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2025/02/10 07:30:06 UTC
[PR] Removed the unused provider's package [airflow]
potiuk opened a new pull request, #46608:
URL: https://github.com/apache/airflow/pull/46608
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2658123498
There are still few errors due to cross-provider dependencies with fab provider / azure paths not fixed - but generally it should be ready for review (I will fix the errors tomorrow).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957074795
##########
pyproject.toml:
##########
@@ -589,14 +596,12 @@ disable_error_code = [
"annotation-unchecked",
]
# Since there are no __init__.py files in
-# providers/src/apache/airflow/providers we need to tell MyPy where the "base"
+# providers/<provided>src/apache/airflow/providers we need to tell MyPy where the "base"
Review Comment:
```suggestion
# providers/<provider>src/apache/airflow/providers we need to tell MyPy where the "base"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957137894
##########
providers/apache/cassandra/tests/system/__init__.py:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore
Review Comment:
No, not possible I am afraid. And it is needed, because with this change we have the same "packages" in different places in source tree. For example when you have:
* `amazon/tests/system/amazon/....`
* `apache/cassandra/tests/system/apache/cassandra`
And you want to do `import system.apache.cassandra` and you have `amazon's` system in the PYTHONPATH first, you will get `no such package system.apache`.
This is because those packages must be declared as "namespace" packages.
The `__path__ = __import__("pkgutil").extend_path(__path__, __name__)` is the canonical way of implementing it (unless you have implicit - or so called native - namespace packages).
What we are using now is so called "legacy namespace packages" - this is described here https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#legacy-namespace-packages
The reason why we are using legacy namespace packages insttead of native ones, is mainly due to compatibiity with airflow package itself. In Airflow we are already using legacy namespace packages for "airflow.providers" - https://github.com/apache/airflow/blob/main/airflow/__init__.py#L26 - and the problem is that you cannot really mix native and legacy packages.
See the quotes from the documentation of namespace packages:
> These two methods, that were used to create namespace packages prior to [PEP 420](https://peps.python.org/pep-0420/), are now considered to be obsolete and should not be used unless you need compatibility with packages already using this method. Also, [pkg_resources](https://setuptools.pypa.io/en/latest/pkg_resources.html) has been deprecated.
And taking into acount this:
> While native namespace packages and pkgutil-style namespace packages are largely compatible, pkg_resources-style namespace packages are not compatible with the other methods. Itβs inadvisable to use different methods in different distributions that provide packages to the same namespace.
We are not supposed to mix the two methods. So we don't. But it also means that we have to make sure that all the packages that are potentially appearing in several places in our source tree are "legacy" namespace packages.
In order to switch fully to native namespace packages (i.e. not having an`__init__.py` at all) - we would have to do something that I strongly advocate for, i.e. stop doing any work while we are importing airlfow. Currently airflow's `__init__.py` does a lot of things - initalizes settings, sqlalchemy, logging and a number of other things.
And we do a lot of workarounds to minimise side effects of it -> lazy loading stuff in a number of places, not initializing ORM in other places. Maybe when we complete Airflow packaging work for Airlfow 3 we will get to the point that this initialisation and lazy loading will be removed and we just explicitly do initialization when we run appropriate "airflow" commands (which is something I advocate for a long time).
This whole "pkgutil" dance we do now could be removed if we do so - and all the packages in airlfow and providers could be "native namespace package" and we could get rid of about 100 `__init__.py` files (including the `airflow.__init__.py` one ) in our repo. We could also stop having the "partially initialized module (most likely due to a circular import)` pesky errors we and a number of our users experience - because having all the initializationi happening in `airflow.__init__.py` is the root cause for those.
See https://packaging.python.org/en/latest/guides/packaging-namespace-packages/ for details on namespace packages.
cc: @ashb @uranusjr @kaxil -> something for consideration in our discussions on how to repackage airlfow soon. I keep on explaining why running code in `airflow.__init__.py` and advocating for removal of it and replace it with explicit initialization, yet that topic have not been discussed yet, but I will plan to start a discussion about it soon once we approach the packaging subject. I am not sure what's your thinking is about this - I know you spent consirderable amount of time on doing all the "lazy initalization" dance all over the places, and I think it adds a lot of complexity to our code and only partially solves the `cicular imports` problem. But I know @ashb has very strong feeling about being able to do "from airlfow import Dag" - which more or less requires all this complexity. I just don't think it's worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660976101
BTW.I plan to have a separate discussion on the "final" layout of the provider test code so I plan to merge it rather quickly and then write up a discussion thread where I would want people to chime in and make some final decisions. I am afraid if I won't merge it quickly some test conflicts will take more than they are worth before. So @o-nikolas @Lee-W -> I think, if the tests pass after rebase, I will merge it, but it is really up to a follow up discussion how the folder and structure and imports will look like eventually (this is all test code so we can remove it again it will be far simpler to do follow-up renames.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957070404
##########
providers/google/tests/provider_tests/google/cloud/hooks/test_cloud_sql.py:
##########
@@ -1596,6 +1596,8 @@ def get_processor():
processor = os.uname().machine
if processor == "x86_64":
processor = "amd64"
+ if processor == "aarch64":
Review Comment:
One of the tests in google was failing on MacOS because we missed the mapping here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957118415
##########
tests_common/test_utils/gcp_system_helpers.py:
##########
Review Comment:
Would this be better placed in google provider in future? (not needed in this PR)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957075522
##########
scripts/ci/pre_commit/update_providers_build_files.py:
##########
@@ -85,10 +60,7 @@ def _find_new_providers_structure() -> None:
# get all folders from arguments
for examined_file in file_list:
- if not examined_file.startswith("providers/src"):
- _find_new_providers_structure()
- else:
- _find_old_providers_structure()
+ _find_new_providers_structure(Path(examined_file))
Review Comment:
I will find and replace "_new_providers" into "providers" and remove all other "_old_providers" in a follow-up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1956753391
##########
dev/example_dags/README.md:
##########
@@ -1,34 +0,0 @@
-<!--
Review Comment:
This had not been used any more for refreshing old example_dags path. We can just remove those.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957137894
##########
providers/apache/cassandra/tests/system/__init__.py:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore
Review Comment:
No, not possible I am afraid. And it is needed, because with this change we have the same "packages" in different places in source tree. For example when you have:
* `amazon/tests/system/amazon/....`
* `apache/cassandra/tests/system/apache/cassandra`
And you want to do `import system.apache.cassandra` and you have `amazon's` system in the PYTHONPATH first, you will get `no such package system.apache`.
This is because those packages must be declared as "namespace" packages.
The `__path__ = __import__("pkgutil").extend_path(__path__, __name__)` is the canonical way of implementing it (unless you have implicit - or so called native - namespace packages).
What we are using now is so called "legacy namespace packages" - this is described here https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#legacy-namespace-packages
The reason why we are using legacy namespace packages insttead of native ones, is mainly due to compatibiity with airflow package itself. In Airflow we are already using legacy namespace packages for "airflow.providers" - https://github.com/apache/airflow/blob/main/airflow/__init__.py#L26 - and the problem is that you cannot really mix native and legacy packages.
See the quotes from the documentation of namespace packages:
> These two methods, that were used to create namespace packages prior to [PEP 420](https://peps.python.org/pep-0420/), are now considered to be obsolete and should not be used unless you need compatibility with packages already using this method. Also, [pkg_resources](https://setuptools.pypa.io/en/latest/pkg_resources.html) has been deprecated.
And taking into acount this:
> While native namespace packages and pkgutil-style namespace packages are largely compatible, pkg_resources-style namespace packages are not compatible with the other methods. Itβs inadvisable to use different methods in different distributions that provide packages to the same namespace.
We are not supposed to mix the two methods. So we don't. But it also means that we have to make sure that all the packages that are potentially appearing in several places in our source tree are "legacy" namespace packages.
In order to switch fully to native namespace packages (i.e. not having an`__init__.py` at all) - we would have to do something that I strongly advocate for, i.e. stop doing any work while we are importing airlfow. Currently airflow's `__init__.py` does a lot of things - initalizes settings, sqlalchemy, logging and a number of other things.
And we do a lot of workarounds to minimise side effects of it -> lazy loading stuff in a number of places, not initializing ORM in other places. Maybe when we complete Airflow packaging work for Airlfow 3 we will get to the point that this initialisation and lazy loading will be removed and we just explicitly do initialization when we run appropriate "airflow" commands (which is something I advocate for a long time).
This whole "pkgutil" dance we do now could be removed if we do so - and all the packages in airlfow and providers could be "native namespace package" and we could get rid of about 100 `__init__.py` files (including the `airflow.__init__.py` one ) in our repo. We could also stop having the `partially initialized module (most likely due to a circular import)` pesky errors we and a number of our users experience - because having all the initializationi happening in `airflow.__init__.py` is the root cause for those.
See https://packaging.python.org/en/latest/guides/packaging-namespace-packages/ for details on namespace packages.
cc: @ashb @uranusjr @kaxil -> something for consideration in our discussions on how to repackage airlfow soon. I keep on explaining why running code in `airflow.__init__.py` and advocating for removal of it and replace it with explicit initialization, yet that topic have not been discussed yet, but I will plan to start a discussion about it soon once we approach the packaging subject. I am not sure what's your thinking is about this - I know you spent consirderable amount of time on doing all the "lazy initalization" dance all over the places, and I think it adds a lot of complexity to our code and only partially solves the `cicular imports` problem. But I know @ashb has very strong feeling about being able to do "from airlfow import Dag" - which more or less requires all this complexity. I just don't think it's worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957075202
##########
scripts/ci/pre_commit/check_providers_subpackages_all_have_init.py:
##########
@@ -30,40 +32,119 @@
"static",
]
+PATH_EXTENSION_STRING = '__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore'
+
+# Here we should add the second level paths that we want to have sub-packages in
+KNOWN_SECOND_LEVEL_PATHS = ["apache", "atlassian", "common", "cncf", "dbt", "microsoft"]
Review Comment:
Yes. I plan to find a way to derive it automatically in a follow-up. But this PR is already too large to try to do it now :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957072969
##########
providers/oracle/pyproject.toml:
##########
@@ -64,7 +64,7 @@ dependencies = [
# Any change in the dependencies is preserved when the file is regenerated
[project.optional-dependencies]
"numpy" = [
- "numpy",
+ "numpy>=1.26.0",
Review Comment:
Again - with isolation, I found we miss lower-binding here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957076352
##########
tests/www/views/test_views_home.py:
##########
@@ -119,86 +118,6 @@ def test_home_status_filter_cookie(admin_client):
assert flask.session[FILTER_LASTRUN_COOKIE] == "all_states"
-@pytest.fixture(scope="module")
Review Comment:
Same - those are "fab" tests essentially.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957070238
##########
providers/google/tests/deprecations_ignore.yml:
##########
@@ -15,17 +15,6 @@
# specific language governing permissions and limitations
# under the License.
---
-
Review Comment:
Those deprecation ignores for Amazon are no longer needed as I found out. And we have now separate deprecations_ignore per provder, and the common autouse fixture will find and apply those deprecations ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957075739
##########
tests/always/test_example_dags.py:
##########
@@ -91,6 +91,7 @@ def get_suspended_providers_folders() -> list[str]:
suspended_providers.append(
provider_path.parent.relative_to(AIRFLOW_SOURCES_ROOT)
.as_posix()
+ # TODO(potiuk): check
Review Comment:
I will have to fix it separately
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957074333
##########
providers/tests/conftest.py:
##########
@@ -1,68 +0,0 @@
-# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Whole `tests` folder is not needed any more, same as `src`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957073966
##########
providers/src/README.md:
##########
@@ -1,44 +0,0 @@
-<!--
Review Comment:
No longer needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957070238
##########
providers/google/tests/deprecations_ignore.yml:
##########
@@ -15,17 +15,6 @@
# specific language governing permissions and limitations
# under the License.
---
-
Review Comment:
Those deprecation ignores for Amazon are no longer needed as I found out. And we have now separate deprecations_ignore per provder, and the common autouse fixture will find and apply those deprecations ignore files always.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1956849973
##########
dev/breeze/tests/test_pytest_args_for_test_types.py:
##########
@@ -25,15 +25,18 @@
# TODO(potiuk): rename to all_providers when we move all providers to the new structure
def _all_new_providers() -> list[str]:
Review Comment:
We can do this rename now, ya?
##########
providers/amazon/tests/system/amazon/aws/example_appflow.py:
##########
@@ -30,6 +28,7 @@
AppflowRunFullOperator,
)
from airflow.providers.standard.operators.bash import BashOperator
+from system.amazon.aws.utils import SystemTestContextBuilder
Review Comment:
Much more succinct, very nice :+1:
##########
docs/conf.py:
##########
@@ -226,7 +226,11 @@
"sphinx_jinja",
]
)
- exclude_patterns = ["operators/_partials"]
+ empty_subpackages = ["apache", "atlassian", "common", "cncf", "dbt", "microsoft"]
Review Comment:
Does Amazon need this treatment given the `aws` subdirectory it has?
##########
scripts/ci/pre_commit/check_providers_subpackages_all_have_init.py:
##########
@@ -30,40 +32,119 @@
"static",
]
+PATH_EXTENSION_STRING = '__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore'
+
+# Here we should add the second level paths that we want to have sub-packages in
+KNOWN_SECOND_LEVEL_PATHS = ["apache", "atlassian", "common", "cncf", "dbt", "microsoft"]
Review Comment:
This list is in a lot of places. If we ever have to add to it, it will be tricky to find them all :grimacing:
##########
pyproject.toml:
##########
@@ -411,6 +414,10 @@ testing = ["dev", "providers.tests", "task_sdk.tests", "tests_common", "tests"]
"tests_common/test_utils/compat.py" = ["TID251", "F401"]
"tests_common/pytest_plugin.py" = ["F811"]
+# While pandas imoport is banned, sql.pyi should be exclueded from it as it does not have TYPE_CHECKING
+# mechanism and whole .pyi is really "type-checking" only
Review Comment:
Nit
```suggestion
# While pandas import is banned, sql.pyi should be excluded from it as it does not have a TYPE_CHECKING
# mechanism and whole .pyi is really "type-checking" only
```
##########
dev/breeze/src/airflow_breeze/utils/run_tests.py:
##########
@@ -293,41 +291,26 @@ def convert_test_type_to_pytest_args(
f"[info]Removing {provider_test_to_exclude} from {providers_with_exclusions}[/]"
)
providers_with_exclusions.remove(provider_test_to_exclude)
- else:
- # TODO(potiuk): remove me when all providers are migrated
- get_console().print(f"[info]Adding {provider_test_to_exclude} to pytest ignores[/]")
- providers_with_exclusions.append(
- "--ignore=providers/tests/" + excluded_provider.replace(".", "/")
- )
return providers_with_exclusions
if test_type.startswith(PROVIDERS_LIST_PREFIX):
provider_list = test_type[len(PROVIDERS_LIST_PREFIX) : -1].split(",")
providers_to_test = []
for provider in provider_list:
- # TODO(potiuk) - remove when all providers are new-style
- provider_path = OLD_TESTS_PROVIDERS_ROOT.joinpath(provider.replace(".", "/")).relative_to(
- AIRFLOW_SOURCES_ROOT
+ provider_path = (
+ AIRFLOW_PROVIDERS_DIR.joinpath(provider.replace(".", "/")).relative_to(
+ AIRFLOW_SOURCES_ROOT
+ )
+ / "tests"
)
if provider_path.is_dir():
providers_to_test.append(provider_path.as_posix())
else:
- # TODO(potiuk) - remove when all providers are new-style
- old_provider_path = provider_path
- provider_path = (
- AIRFLOW_PROVIDERS_DIR.joinpath(provider.replace(".", "/")).relative_to(
- AIRFLOW_SOURCES_ROOT
- )
- / "tests"
+ get_console().print(
+ f"[error]Neither {provider_path} exist for {provider} "
+ "- which means that provider has no tests. This is bad idea. "
+ "Please add it (all providers should have a package in tests)"
Review Comment:
We are only looking under one path now, so we can update the language of this message:
```suggestion
f"[error] {provider_path} does not exist for {provider} "
"- which means that this provider has no tests. This is bad idea. "
"Please add it (all providers should have at least a package in tests)."
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957072142
##########
providers/microsoft/azure/tests/conftest.py:
##########
@@ -89,109 +69,10 @@ def wrapper(*conns: T):
@pytest.fixture
def mocked_connection(request, create_mock_connection):
- """Helper indirect fixture for create test connection."""
+ """Create test connection."""
return create_mock_connection(request.param)
-def mock_connection(schema: str | None = None, host: str | None = None):
Review Comment:
Moved all those from conftest to `test_utils`. Those functions are imported in tests and importing stuff from conftest.py is not going to work as you would just impot "from conftest" - this is also not the intention of conftest.py -> conftest.py is automatically read by pytest and fixtures are configured and made available from it - it was never supposed to be used to import things from.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957108157
##########
providers/apache/cassandra/tests/system/__init__.py:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore
Review Comment:
What is this path extension for?
Is it possible to make this w/o type ignote? :-D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957131863
##########
dev/breeze/tests/test_pytest_args_for_test_types.py:
##########
@@ -25,15 +25,18 @@
# TODO(potiuk): rename to all_providers when we move all providers to the new structure
def _all_new_providers() -> list[str]:
Review Comment:
I prefer to do it next... It's already FAR too many changes :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957139221
##########
tests_common/test_utils/gcp_system_helpers.py:
##########
Review Comment:
Yes. We should move it. And yes. Not in this PR :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #46608:
URL: https://github.com/apache/airflow/pull/46608
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957070053
##########
providers/google/pyproject.toml:
##########
@@ -143,7 +143,8 @@ dependencies = [
# Any change in the dependencies is preserved when the file is regenerated
[project.optional-dependencies]
"apache.beam" = [
- "apache-beam[gcp]",
+ 'apache-beam[gcp]>=2.53.0; python_version < "3.12"',
Review Comment:
One of the tests started failing when we isolated apache-beam and it turned out that we did not have the same limits set here as in apache.beam provider. That's another side-effect of better isolation between providers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957074989
##########
pyproject.toml:
##########
@@ -636,8 +641,7 @@ ignore_errors = true
[dependency-groups]
dev = [
- # TODO(potiuk): remove me when all providers are moved to new structure
- "local-providers",
+ "apache-airflow",
Review Comment:
We need to ad dev dependency group from airflow and remvoe the "local-provider" one - this way pytest and others will be installed automatically when runnign `uv sync`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957116368
##########
pyproject.toml:
##########
@@ -377,6 +373,9 @@ testing = ["dev", "providers.tests", "task_sdk.tests", "tests_common", "tests"]
"docker_tests/*" = ["D", "TID253", "S101", "TRY002"]
"kubernetes_tests/*" = ["D", "TID253", "S101", "TRY002"]
"helm_tests/*" = ["D", "TID253", "S101", "TRY002"]
+"providers/**/tests/*" = ["D", "TID253", "S101", "TRY002"]
+# exclude assert in edge provider
+"providers/edge/src/airflow/providers/edge/example_dags/integration_test.py" = ["S101"]
Review Comment:
Oh, that seems I need a follow-up cleaning to remove this :-D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660956602
> Just a few sanity check comments, compared to Niko I did not find any glitches. Good to be merged after nit's applied.
π π π π π π π π
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957137894
##########
providers/apache/cassandra/tests/system/__init__.py:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore
Review Comment:
No, not possible I am afraid. And it is needed, because with this change we have the same "packages" in different places in source tree. For example when you have:
* `amazon/tests/system/amazon/....`
* `apache/cassandra/tests/system/apache/cassandra`
And you want to do `import system.apache.cassandra` and you have `amazon's` system in the PYTHONPATH first, you will get `no such package system.apache`.
This is because those packages must be declared as "namespace" packages.
The `__path__ = __import__("pkgutil").extend_path(__path__, __name__)` is the canonical way of implementing it (unless you have implicit - or so called native - namespace packages).
What we are using now is so called "legacy namespace packages" - this is described here https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#legacy-namespace-packages
The reason why we are using legacy namespace packages insttead of native ones, is mainly due to compatibiity with airflow package itself. In Airflow we are already using legacy namespace packages for "airflow.providers" - https://github.com/apache/airflow/blob/main/airflow/__init__.py#L26 - and the problem is that you cannot really mix native and legacy packages.
See the quotes from the documentation of namespace packages:
> These two methods, that were used to create namespace packages prior to [PEP 420](https://peps.python.org/pep-0420/), are now considered to be obsolete and should not be used unless you need compatibility with packages already using this method. Also, [pkg_resources](https://setuptools.pypa.io/en/latest/pkg_resources.html) has been deprecated.
And taking into acount this:
> While native namespace packages and pkgutil-style namespace packages are largely compatible, pkg_resources-style namespace packages are not compatible with the other methods. Itβs inadvisable to use different methods in different distributions that provide packages to the same namespace.
We are not supposed to mix the two methods. So we don't. But it also means that we have to make sure that all the packages that are potentially appearing in several places in our source tree are "legacy" namespace packages.
In order to switch fully to native namespace packages (i.e. not having an`__init__.py` at all) - we would have to do something that I strongly advocate for, i.e. stop doing any work while we are importing airlfow. Currently airflow's `__init__.py` does a lot of things - initalizes settings, sqlalchemy, logging and a number of other things.
And we do a lot of workarounds to minimise side effects of it -> lazy loading stuff in a number of places, not initializing ORM in other places. Maybe when we complete Airflow packaging work for Airlfow 3 we will get to the point that this initialisation and lazy loading will be removed and we just explicitly do initialization when we run appropriate "airflow" commands (which is something I advocate for a long time).
This whole "pkgutil" dance we do now could be removed if we do so - and all the packages in airlfow and providers could be "native namespace package" and we could get rid of about 100 `__init__.py` files (including the `airflow.__init__.py` one ) in our repo. We could also stop having the `partially initialized module (most likely due to a circular import)` pesky errors we and a number of our users experience - because having all the initialization happening in `airflow.__init__.py` is the root cause for those.
See https://packaging.python.org/en/latest/guides/packaging-namespace-packages/ for details on namespace packages.
cc: @ashb @uranusjr @kaxil -> something for consideration in our discussions on how to repackage airlfow soon. I keep on explaining why running code in `airflow.__init__.py` and advocating for removal of it and replace it with explicit initialization, yet that topic have not been discussed yet, but I will plan to start a discussion about it soon once we approach the packaging subject. I am not sure what's your thinking is about this - I know you spent consirderable amount of time on doing all the "lazy initalization" dance all over the places, and I think it adds a lot of complexity to our code and only partially solves the `cicular imports` problem. But I know @ashb has very strong feeling about being able to do "from airlfow import Dag" - which more or less requires all this complexity. I just don't think it's worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957075522
##########
scripts/ci/pre_commit/update_providers_build_files.py:
##########
@@ -85,10 +60,7 @@ def _find_new_providers_structure() -> None:
# get all folders from arguments
for examined_file in file_list:
- if not examined_file.startswith("providers/src"):
- _find_new_providers_structure()
- else:
- _find_old_providers_structure()
+ _find_new_providers_structure(Path(examined_file))
Review Comment:
I will find and replace "_new_providers" and remove all other "_old_providers" in a follow-up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1956750563
##########
dev/breeze/src/airflow_breeze/utils/run_tests.py:
##########
@@ -138,18 +137,7 @@ def test_paths(test_type: str, backend: str) -> tuple[str, str, str]:
def get_ignore_switches_for_provider(provider_folders: list[str]) -> list[str]:
args = []
for providers in provider_folders:
- args.extend(
- [
- # TODO(potiuk): remove the old ways once we migrate all providers to the new structure
- f"--ignore=providers/tests/{providers}",
- f"--ignore=providers/tests/system/{providers}",
- f"--ignore=providers/tests/integration/{providers}",
- # New structure
- f"--ignore=providers/{providers}/tests/",
- f"--ignore=providers/{providers}/tests/system",
- f"--ignore=providers/{providers}/tests/integration",
- ]
- )
+ args.append(f"--ignore=providers/{providers}/tests/")
Review Comment:
We can finally exclude one dir wth all test types, "per provider"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957112551
##########
providers/edge/docs/install_on_windows.rst:
##########
@@ -55,7 +55,7 @@ To setup a instance of Edge Worker on Windows, you need to follow the steps belo
7. Create a new folder ``dags`` in ``C:\\Airflow`` and copy the relevant DAG files in it.
(At least the DAG files which should be executed on the edge alongside the dependencies. For testing purposes
the DAGs from the ``apache-airflow`` repository can be used located in
- <https://github.com/apache/airflow/tree/main/providers/src/airflow/providers/edge/example_dags>.)
+ <https://github.com/apache/airflow/tree/main/providers/edge/src/airflow/providers/edge/example_dags>.)
Review Comment:
Good catch! Missed this during move..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660233535
Would it be possible to split this PR? This is really a "beast" to review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957072477
##########
providers/microsoft/mssql/tests/conftest.py:
##########
@@ -16,43 +16,4 @@
# under the License.
from __future__ import annotations
-import pathlib
-import re
-from inspect import currentframe
-from os.path import dirname, join, realpath
-
-import pytest
-
pytest_plugins = "tests_common.pytest_plugin"
-
-
-@pytest.hookimpl(tryfirst=True)
-def pytest_configure(config: pytest.Config) -> None:
- deprecations_ignore_path = pathlib.Path(__file__).parent.joinpath("deprecations_ignore.yml")
- dep_path = [deprecations_ignore_path] if deprecations_ignore_path.exists() else []
- config.inicfg["airflow_deprecations_ignore"] = (
- config.inicfg.get("airflow_deprecations_ignore", []) + dep_path # type: ignore[assignment,operator]
- )
-
-
-def remove_license_header(content: str) -> str:
Review Comment:
Same here - those methods are moved to "test_utils" inside the provider.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957075341
##########
scripts/ci/pre_commit/check_providers_subpackages_all_have_init.py:
##########
@@ -30,40 +32,119 @@
"static",
]
+PATH_EXTENSION_STRING = '__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore'
Review Comment:
All the `__init__.py` files in this PR are checked and regenerated as needed automatically by this pre-commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957069544
##########
providers/fab/tests/provider_tests/fab/www/views/test_views_acl.py:
##########
@@ -85,6 +94,25 @@
}
+def _get_appbuilder_pk_string(model_view_cls, instance) -> str:
Review Comment:
I moved a number of tests from "airflow/www" to "fab" -> because those tests are testing permission models for FAB rather than views themselves and they are using some "fab-specific" api connection utils (create user and such) wich should no longer be used from "airlfow" core once the "fab" provicder tests code is truly isolated from airflow core. There was already a number of tests like that moved before but this changed allowed to find and detect remaining ones, and as a side-effect airflow tests should no longer (except some structural tests that auto-detec provider example dags and import it) import anything from FAB. cc: @vincbeck
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957132406
##########
docs/conf.py:
##########
@@ -226,7 +226,11 @@
"sphinx_jinja",
]
)
- exclude_patterns = ["operators/_partials"]
+ empty_subpackages = ["apache", "atlassian", "common", "cncf", "dbt", "microsoft"]
Review Comment:
No. Because it does not have "amazon-aws" provider, only "amazon" provider. The others are separate providers, but if any code is added to "amazon.something_else" - they will still be in "amazon" provider. Unless we change the approach and have separate "amazon-aws" provider. This is what we did with "jira" provider that became "atlassian-jira" in the past. But for now, this is still single "apache-airflow-providers-amazon" provider and as such it cannot (and need not) be added here - mostly because anything "under" amazon is always in the same source tree - never in separate "amazon" packages in different providers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660318350
> > BTW. It is finally GREEN....
>
> Promise I'll review by tomorrow (If I'm not getting sick like the rest of the family) but need a fresh mind for such an amount of code :-D
I will do a self-review myself and add some comment explanations to make it easier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1956749026
##########
.github/workflows/special-tests.yml:
##########
@@ -218,7 +218,7 @@ jobs:
runs-on-as-json-default: ${{ inputs.runs-on-as-json-default }}
test-name: "SystemTest"
test-scope: "System"
- test-groups: ${{ inputs.test-groups }}
+ test-groups: "['core']"
Review Comment:
Here - only "core" system tests are run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957069868
##########
providers/apache/cassandra/tests/system/apache/__init__.py:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
All those `__init__.py` files are automatically generated by the pre-commit we have that autmatically adds __init__.py as needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957076180
##########
tests/www/views/conftest.py:
##########
@@ -140,79 +136,3 @@ def anonymous_client(app):
@pytest.fixture
def anonymous_client_as_admin(app):
return client_without_login_as_admin(app)
-
Review Comment:
moved to "tests_common" - those are now used in both "airflow" and "fab provider" tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957073262
##########
providers/pyproject.toml:
##########
@@ -1,93 +0,0 @@
-# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
There is no longer separate "providers" distribution. We only have individual provider's distributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957076467
##########
tests/www/views/test_views_trigger_dag.py:
##########
@@ -298,26 +294,6 @@ def test_trigger_endpoint_uses_existing_dagbag(admin_client):
check_content_in_response("example_bash_operator", resp)
-def test_viewer_cant_trigger_dag(app):
Review Comment:
FAB tests as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957076413
##########
tests/www/views/test_views_tasks.py:
##########
@@ -137,34 +131,6 @@ def _init_dagruns(app):
clear_db_xcom()
-@pytest.fixture(scope="module")
Review Comment:
Same here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957117826
##########
scripts/ci/pre_commit/update_providers_build_files.py:
##########
@@ -85,10 +60,7 @@ def _find_new_providers_structure() -> None:
# get all folders from arguments
for examined_file in file_list:
- if not examined_file.startswith("providers/src"):
- _find_new_providers_structure()
- else:
- _find_old_providers_structure()
+ _find_new_providers_structure(Path(examined_file))
Review Comment:
Nit: Can you add a todo then into code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660273022
> Would it be possible to split this PR? This is really a "beast" to review.
Unfortunately ... I am afraid it's not easy. I thpught about it, but I was not able to figure out how to split it.
There are many connected things and a bit of "chain reaction" -> removing of providers tests requires to move imports, which (for example cc: @vincbeck ) make us move all the remaining "core" tests that accessed FAB permission model to be moved to fab, it also makes it necessary to move integration tests there etc., etc. ...
So - as much I would love to, I am afraid it willl be very difficult if not impossible.
The good thing about it is that there are many removals and similar changes across the board.
I am really sorry for the reviewers, but I am afraid we have to bite the bullet.
BTW. It is finally GREEN....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1956754612
##########
providers/airbyte/tests/conftest.py:
##########
@@ -16,17 +16,4 @@
# under the License.
from __future__ import annotations
-import pathlib
Review Comment:
All other `conftest.py` removals in 90+ providers follow the same pattern. We are using warning deprecation impleented in the `pytest_plugin`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1956748670
##########
.github/workflows/integration-system-tests.yml:
##########
@@ -197,8 +197,7 @@ jobs:
use-uv: ${{ inputs.use-uv }}
- name: "System Tests"
run: >
- ./scripts/ci/testing/run_system_tests.sh
- tests/system/example_empty.py providers/tests/system/example_empty.py
+ ./scripts/ci/testing/run_system_tests.sh tests/system/example_empty.py
Review Comment:
We do not distinguish "core" and "providers" system tests any more. They are essentially the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957076952
##########
tests_common/pytest_plugin.py:
##########
@@ -1712,3 +1716,15 @@ def create_db_api_hook(request):
test_db_hook.escape_column_names = escape_column_names or False
return test_db_hook
+
+
+@pytest.fixture(autouse=True, scope="session")
Review Comment:
This is how we make sure that "tests" folders are root you can import from "from integration.apache.beam" ....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957076062
##########
tests/conftest.py:
##########
@@ -44,14 +44,6 @@
]
-@pytest.hookimpl(tryfirst=True)
Review Comment:
Moved to "tests_common"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957114344
##########
providers/oracle/pyproject.toml:
##########
@@ -64,7 +64,7 @@ dependencies = [
# Any change in the dependencies is preserved when the file is regenerated
[project.optional-dependencies]
"numpy" = [
- "numpy",
+ "numpy>=1.26.0",
Review Comment:
Ah, yeah also had this in example dags, Python 3.12 needs 1.28 at least. Older Python accepts also older stuff.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957132656
##########
scripts/ci/pre_commit/update_providers_build_files.py:
##########
@@ -85,10 +60,7 @@ def _find_new_providers_structure() -> None:
# get all folders from arguments
for examined_file in file_list:
- if not examined_file.startswith("providers/src"):
- _find_new_providers_structure()
- else:
- _find_old_providers_structure()
+ _find_new_providers_structure(Path(examined_file))
Review Comment:
I will have the PR VERY soon after merging this one. So no worries it will be gone in no time at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2661007869
Merged !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660288580
> BTW. It is finally GREEN....
Promise I'll review by tomorrow (If I'm not getting sick like the rest of the family) but need a fresh mind for such an amount of code :-D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1956829089
##########
providers/amazon/tests/system/amazon/aws/example_mongo_to_s3.py:
##########
@@ -16,14 +16,13 @@
# under the License.
from __future__ import annotations
-from providers.amazon.tests.system.amazon.aws.utils import SystemTestContextBuilder
-
from airflow.models.baseoperator import chain
from airflow.models.dag import DAG
from airflow.providers.amazon.aws.operators.s3 import S3CreateBucketOperator, S3DeleteBucketOperator
from airflow.providers.amazon.aws.transfers.mongo_to_s3 import MongoToS3Operator
from airflow.utils.timezone import datetime
from airflow.utils.trigger_rule import TriggerRule
+from system.amazon.aws.utils import SystemTestContextBuilder
Review Comment:
Most of similar changes are result of applyin PEP-8 import order on imports afre we cnanged the "from"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660450584
Having a look now! I won't have time to finish it entirely, but I'll get a start :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957075921
##########
tests/always/test_project_structure.py:
##########
@@ -56,48 +53,86 @@ def assert_file_contains(self, filename: pathlib.Path, pattern: str):
def test_providers_modules_should_have_tests(self):
"""
- Assert every module in /providers/src/airflow/providers has a corresponding test_ file in providers/providers.
+ Assert every module in /providers/ has a corresponding test_ file in providers/providers.
"""
# The test below had a but for quite a while and we missed a lot of modules to have tess
# We should make sure that one goes to 0
+ # TODO(potiuk) - check if that test actually tests something
OVERLOOKED_TESTS = [
- "providers/tests/amazon/aws/executors/batch/test_boto_schema.py",
- "providers/tests/amazon/aws/executors/batch/test_batch_executor_config.py",
- "providers/tests/amazon/aws/executors/batch/test_utils.py",
- "providers/tests/amazon/aws/executors/ecs/test_boto_schema.py",
- "providers/tests/amazon/aws/executors/ecs/test_ecs_executor_config.py",
- "providers/tests/amazon/aws/executors/ecs/test_utils.py",
- "providers/tests/amazon/aws/executors/utils/test_base_config_keys.py",
- "providers/tests/amazon/aws/operators/test_emr.py",
- "providers/tests/amazon/aws/operators/test_sagemaker.py",
- "providers/tests/amazon/aws/sensors/test_emr.py",
- "providers/tests/amazon/aws/sensors/test_sagemaker.py",
- "providers/tests/amazon/aws/test_exceptions.py",
- "providers/tests/amazon/aws/triggers/test_step_function.py",
- "providers/tests/amazon/aws/utils/test_rds.py",
- "providers/tests/amazon/aws/utils/test_sagemaker.py",
- "providers/tests/amazon/aws/waiters/test_base_waiter.py",
- "providers/tests/apache/drill/operators/test_drill.py",
- "providers/tests/apache/druid/operators/test_druid_check.py",
- "providers/tests/apache/hdfs/hooks/test_hdfs.py",
- "providers/tests/apache/hdfs/log/test_hdfs_task_handler.py",
- "providers/tests/apache/hdfs/sensors/test_hdfs.py",
- "providers/tests/apache/hive/plugins/test_hive.py",
- "providers/tests/celery/executors/test_celery_executor_utils.py",
- "providers/tests/celery/executors/test_default_celery.py",
- "providers/tests/cloudant/test_cloudant_fake.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/executors/batch/test_batch_executor_config.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/executors/batch/test_boto_schema.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/executors/batch/test_utils.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/executors/ecs/test_boto_schema.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/executors/ecs/test_ecs_executor_config.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/executors/ecs/test_utils.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/executors/utils/test_base_config_keys.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/operators/test_emr.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/operators/test_sagemaker.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/sensors/test_emr.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/sensors/test_sagemaker.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/test_exceptions.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/triggers/test_step_function.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/utils/test_rds.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/utils/test_sagemaker.py",
+ "providers/amazon/tests/provider_tests/amazon/aws/waiters/test_base_waiter.py",
+ "providers/amazon/tests/provider_tests/amazon/test_version_compat.py",
+ "providers/apache/hdfs/tests/provider_tests/apache/hdfs/hooks/test_hdfs.py",
+ "providers/apache/hdfs/tests/provider_tests/apache/hdfs/log/test_hdfs_task_handler.py",
+ "providers/apache/hdfs/tests/provider_tests/apache/hdfs/sensors/test_hdfs.py",
+ "providers/apache/hive/tests/provider_tests/apache/hive/plugins/test_hive.py",
+ "providers/celery/tests/provider_tests/celery/executors/test_celery_executor_utils.py",
+ "providers/celery/tests/provider_tests/celery/executors/test_default_celery.py",
+ "providers/celery/tests/provider_tests/celery/test_version_compat.py",
+ "providers/cloudant/tests/provider_tests/cloudant/test_cloudant_fake.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/executors/test_kubernetes_executor_types.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/executors/test_kubernetes_executor_utils.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/operators/test_kubernetes_pod.py",
+ "providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/test_exceptions.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/test_k8s_model.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/test_kube_client.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/test_kube_config.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/test_python_kubernetes_script.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/test_secret.py",
+ "providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/test_version_compat.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/triggers/test_kubernetes_pod.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/utils/test_delete_from.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/utils/test_k8s_hashlib_wrapper.py",
"providers/cncf/kubernetes/tests/provider_tests/cncf/kubernetes/utils/test_xcom_sidecar.py",
+ "providers/common/compat/tests/provider_tests/common/compat/lineage/test_entities.py",
Review Comment:
This check was not working after the move to "providers" folder. It starts to work back and I had to add all the modules that have no corresponding tests. We might want to rethink how this works (but that would be a separate discussion/PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957076282
##########
tests/www/views/test_views_dagrun.py:
##########
@@ -20,80 +20,18 @@
import pytest
from airflow.models import DagBag, DagRun, TaskInstance
-from airflow.security import permissions
from airflow.utils import timezone
from airflow.utils.session import create_session
from airflow.utils.types import DagRunTriggeredByType, DagRunType
-from airflow.www.views import DagRunModelView
-from providers.fab.tests.provider_tests.fab.auth_manager.api_endpoints.api_connexion_utils import (
- create_user,
- delete_roles,
- delete_user,
-)
-from tests.www.views.test_views_tasks import _get_appbuilder_pk_string
from tests_common.test_utils.www import (
check_content_in_response,
check_content_not_in_response,
- client_with_login,
)
pytestmark = pytest.mark.db_test
-@pytest.fixture(scope="module")
Review Comment:
Moved to FAB provider where they belong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957076567
##########
tests/www/views/test_views_variable.py:
##########
@@ -49,30 +46,6 @@ def _clear_variables():
session.query(Variable).delete()
-@pytest.fixture(scope="module")
Review Comment:
Also FAB tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660842629
For those who will be reviewing this one - I recommend `git diff` -> UI of Github is very, very slow with that huge of a change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #46608:
URL: https://github.com/apache/airflow/pull/46608#discussion_r1957137894
##########
providers/apache/cassandra/tests/system/__init__.py:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+__path__ = __import__("pkgutil").extend_path(__path__, __name__) # type: ignore
Review Comment:
No, not possible I am afraid. And it is needed, because with this change we have the same "packages" in different places in source tree. For example when you have:
* `amazon/tests/system/amazon/....`
* `apache/cassandra/tests/system/apache/cassandra`
And you want to do `import system.apache.cassandra` and you have `amazon's` system in the PYTHONPATH first, you will get `no such package system.apache`.
This is because those packages must be declared as "namespace" packages.
The `__path__ = __import__("pkgutil").extend_path(__path__, __name__)` is the canonical way of implementing it (unless you have implicit - or so called native - namespace packages).
What we are using now is so called "legacy namespace packages" - this is described here https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#legacy-namespace-packages
The reason why we are using legacy namespace packages insttead of native ones, is mainly due to compatibiity with airflow package itself. In Airflow we are already using legacy namespace packages for "airflow.providers" - https://github.com/apache/airflow/blob/main/airflow/__init__.py#L26 - and the problem is that you cannot really mix native and legacy packages.
See the quotes from the documentation of namespace packages:
> These two methods, that were used to create namespace packages prior to [PEP 420](https://peps.python.org/pep-0420/), are now considered to be obsolete and should not be used unless you need compatibility with packages already using this method. Also, [pkg_resources](https://setuptools.pypa.io/en/latest/pkg_resources.html) has been deprecated.
And taking into acount this:
> While native namespace packages and pkgutil-style namespace packages are largely compatible, pkg_resources-style namespace packages are not compatible with the other methods. Itβs inadvisable to use different methods in different distributions that provide packages to the same namespace.
We are not supposed to mix the two methods. So we don't. But it also means that we have to make sure that all the packages that are potentially appearing in several places in our source tree are "legacy" namespace packages.
In order to switch fully to native namespace packages (i.e. not having an`__init__.py` at all) - we would have to do something that I strongly advocate for, i.e. stop doing any work while we are importing airlfow. Currently airflow's `__init__.py` does a lot of things - initalizes settings, sqlalchemy, logging and a number of other things.
And we do a lot of workarounds to minimise side effects of it -> lazy loading stuff in a number of places, not initializing ORM in other places. Maybe when we complete Airflow packaging work for Airlfow 3 we will get to the point that this initialisation and lazy loading will be removed and we just explicitly do initialization when we run appropriate "airflow" commands (which is something I advocate for a long time).
This whole "pkgutil" dance we do now could be removed if we do so - and all the packages in airlfow and providers could be "native namespace package" and we could get rid of about 100 `__init__.py` files (including the `airflow.__init__.py` one ) in our repo. We could also stop having the `partially initialized module (most likely due to a circular import)` pesky errors we and a number of our users experience - because having all the initialization happening in `airflow.__init__.py` is the root cause for those.
See https://packaging.python.org/en/latest/guides/packaging-namespace-packages/ for details on namespace packages.
cc: @ashb @uranusjr @kaxil -> something for consideration in our discussions on how to repackage airlfow soon. I keep on explaining why running code in `airflow.__init__.py` is a bad idea and advocating for removal of it and replace it with explicit initialization, yet that topic have not been discussed yet, but I will plan to start a discussion about it soon once we approach the packaging subject. I am not sure what's your thinking is about this - I know you spent consirderable amount of time on doing all the "lazy initalization" dance all over the places, and I think it adds a lot of complexity to our code and only partially solves the `cicular imports` problem. But I know @ashb has very strong feeling about being able to do "from airlfow import Dag" - which more or less requires all this complexity. I just don't think it's worth it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removed the unused provider's package [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #46608:
URL: https://github.com/apache/airflow/pull/46608#issuecomment-2660974836
Rebased and applied the nits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org