You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/09/04 22:14:16 UTC
[GitHub] [airflow] potiuk opened a new pull request, #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
potiuk opened a new pull request, #26153:
URL: https://github.com/apache/airflow/pull/26153
The conrib modules are carried from Airflow 1.10 and we cannot
remove the classes that were used there without bumping Airlfow
to 3, however we can replace the files with dynamic attribute
retrieval following PEP-562. This way we will have far less number
of files in contrib and users will not get auto-import and
autocomplete when they will be developing their DAGs.
This does not break compatibility but it providers much stronger
signal to the users that they should switch to new classes:
* they will not see the classes in autocomplete and autoimport
any more
* it will seem that the classes are not existing in contrib until
they are used
* if anyone performs static analysis on the DAGs, the analysis
will fail (and this is an intended behaviour)
<!--
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/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962781850
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Tried it, and it does not work, module loader does not use `__getattr__`, it uses much more complex machinery to find modules. By default (with defalt modulespec and loader) it expects the Package object (which should be added to `sys.modules[package])' to return __path__ attribiute (list of paths to search). I event went as far as creating this dynamically created package.
I think we would have to hack and change the loader implementation to not search in __path__ but implement ModuleSpec based on https://peps.python.org/pep-0451 I believe. This would also eventually enable complete removal of sub-packages in contrib, but I somehow feel it's not worth it as it's a bit too magical IMHO and we do not need the whole machinery to reload the modules which is behind it).
The nice thing about this solution is that it's fairly straightforward to get how it works and it's pretty much based on standards (PEP 562 and the fact that all modules should land in sys.modules).
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962781850
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Tried it, and it does not work, module loader does not use `__getattr__`, it uses much more complex machinery to find modules. By default (with defalt modulespec and loader) it expects the Package object (which should be added to `sys.modules[package])' to return `__path__` attribiute (list of paths to search). I even went as far as creating this dynamically created package but then I have no `__paths__` to provide it to :).
I think we would have to hack and change the loader implementation to not search in `__path__` but implement ModuleSpec based on https://peps.python.org/pep-0451 I believe. This would also eventually enable complete removal of sub-packages in contrib, but I somehow feel it's not worth it as it's a bit too magical IMHO and we do not need the whole machinery to reload the modules which is behind it).
The nice thing about this solution is that it's fairly straightforward to get how it works and it's pretty much based on simple standards (PEP 562 and the fact that all modules should land in sys.modules).
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962801661
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Oh yeah, totally not worth building a custom loader mechanism for this!
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962781850
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Tried it, and it does not work, module loader does not use `__getattr__`, it uses much more complex machinery to find modules. By default (with defalt modulespec and loader) it expects the Package object (which should be added to `sys.modules[package])' to return `__path__` attribiute (list of paths to search). I even went as far as creating this dynamically created package but then I have no `__paths__` to provide it to :).
I think we would have to hack and change the loader implementation to not search in `__path__` but implement ModuleSpec based on https://peps.python.org/pep-0451 I believe. This would also eventually enable complete removal of sub-packages in contrib, but I somehow feel it's not worth it as it's a bit too magical IMHO and we do not need the whole machinery to reload the modules which is behind it).
The nice thing about this solution is that it's fairly straightforward to get how it works and it's pretty much based on standards (PEP 562 and the fact that all modules should land in sys.modules).
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962781850
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Tried it, and it does not work, module loader does not use `__getattr__`, it uses much more complex machinery to find modules. By default (with defalt modulespec and loader) it expects the Package object (which should be added to `sys.modules[package])' to return `__path__` attribiute (list of paths to search). I even went as far as creating this dynamically created package but then I have no `__paths__` to provide it to :).
I think we would have to hack and change the loader implementation to not search in __path__ but implement ModuleSpec based on https://peps.python.org/pep-0451 I believe. This would also eventually enable complete removal of sub-packages in contrib, but I somehow feel it's not worth it as it's a bit too magical IMHO and we do not need the whole machinery to reload the modules which is behind it).
The nice thing about this solution is that it's fairly straightforward to get how it works and it's pretty much based on standards (PEP 562 and the fact that all modules should land in sys.modules).
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962781850
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Tried it, and it does not work, module loader does not use __getattr__ it uses much more complex machinery to find modules. By default (with defalt modulespec and loader) it expects the Package object (which should be added to `sys.modules[package])' to return __path__ attribiute (list of paths to search). I event went as far as creating this dynamically created package.
I think we would have to hack and change the loader implementation to not search in __path__ but implement ModuleSpec based on https://peps.python.org/pep-0451 I believe. This would also eventually enable complete removal of sub-packages in contrib, but I somehow feel it's not worth it as it's a bit too magical IMHO and we do not need the whole machinery to reload the modules which is behind it).
The nice thing about this solution is that it's fairly straightforward to get how it works and it's pretty much based on standards (PEP 562 and the fact that all modules should land in sys.modules).
--
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
[GitHub] [airflow] potiuk commented on pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26153:
URL: https://github.com/apache/airflow/pull/26153#issuecomment-1236426362
I attempted to replace all the contrib classes with PEP-562 ones. Looking forward to comments.
I left the tool that I used to generate "__deprecated_classas" dictionaries as there are quite a number of non-contrib deprecations we could remove as next step, so it might be handy. But I did not want to apply it just yet to the other files - they need a bit more selective approach, so I wanted to make sure the approach is cool.
Also the presence of the tool might be good as part of review - to see that I have not missed anything. This change is -10K lines, so likely easier to review the "method", than the result. All the __deprecated_classes were generated using this tool.
Few things I would like to get comments on (It's the first time I apply PEP-562):
* I only dynamically generate modules and classes in the modules. I left packages in "contrib" folders as they were. Likely it would be possible to also dynamically generate packages - but this is beyond PEP-562 andI think this is a good balance between number of files/lines left and "explicitness" of the solution. But if others have comments / experience here to generate also packages, I woudl love to hear it.
* I had to remove all the tests for the classes moved to __deprecated_classes. The reason is that our tests attempt to reload the modules, and fail with those errors:
```
ModuleNotFoundError: spec not found for the module 'airflow.contrib.operators.s3_to_gcs_operator'
```
And if you think of it - it makes sense. We generated the modules out of thin air, so they do not have spec to use to reload them. And those tests are I think quite a bit redundant now - since we know the warnings are generated by ``__getattr__`` and we can test it manually (I did), there is no reason it should fail for the others. So keeping extra tests for those seems quite redundant.
--
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
[GitHub] [airflow] potiuk commented on pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26153:
URL: https://github.com/apache/airflow/pull/26153#issuecomment-1236857217
This is what we get 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
[GitHub] [airflow] potiuk merged pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #26153:
URL: https://github.com/apache/airflow/pull/26153
--
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
[GitHub] [airflow] potiuk commented on pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26153:
URL: https://github.com/apache/airflow/pull/26153#issuecomment-1236428170
BTW. This loooks really cool:
<img width="457" alt="Screenshot 2022-09-05 at 00 39 39" src="https://user-images.githubusercontent.com/595491/188336139-f741c3aa-0ba0-425e-9459-361959ed0c43.png">
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962781850
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Tried it, and it does not work, module loader does not use __getattr__ it uses much more complex machinery to find modules. By default it expects the Package object (whcih should be added to `sys.modules[package])' to return __path__ attribiute (list of paths to search). I event went as far as creating this dynamically created package.
I think we would have to hack and change the loader implementation to not search in __path__ but implement ModuleSpec based on https://peps.python.org/pep-0451 I believe. This would also eventually enable complete removal of sub-packages in contrib, but I somehow feel it's not worth it as it's a bit too magical IMHO and we do not need the whole machinery to reload the modules which is behind it).
The nice thing about this solution is that it's fairly straightforward to get how it works and it's pretty much based on standards (PEP 562 and the fact that all modules should land in sys.modules).
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962763291
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Not sure if it works, but can you put a `__getattr__` on ``airflow.contrib.hooks`` that "loads" each module on demand instead?
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962783194
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
BTW. I updated the naming in the proposed solution - replacing module with package and submodule with module :facepalm:
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962781850
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Tried it, and it does not work, module loader does not use __getattr__ it uses much more complex machinery to find modules. By default (with defalt modulespec and loader) it expects the Package object (whcih should be added to `sys.modules[package])' to return __path__ attribiute (list of paths to search). I event went as far as creating this dynamically created package.
I think we would have to hack and change the loader implementation to not search in __path__ but implement ModuleSpec based on https://peps.python.org/pep-0451 I believe. This would also eventually enable complete removal of sub-packages in contrib, but I somehow feel it's not worth it as it's a bit too magical IMHO and we do not need the whole machinery to reload the modules which is behind it).
The nice thing about this solution is that it's fairly straightforward to get how it works and it's pretty much based on standards (PEP 562 and the fact that all modules should land in sys.modules).
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962734222
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Yeah. It does. That was my worry too.
But I think this does not really matter - those are super small modules (just __getattr__ methods and the dictionaries) and i would say price to pay to access contrib for the users :). We WANT the friction to be there :).
Do you think it's worth to do it differently ?
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #26153: Removed deprecated contrib files and replace them with PEP-562 getattr
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #26153:
URL: https://github.com/apache/airflow/pull/26153#discussion_r962730740
##########
airflow/utils/deprecation_tools.py:
##########
@@ -0,0 +1,46 @@
+# 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.
+
+import functools
+import importlib
+import sys
+import warnings
+from types import ModuleType
+from typing import Dict
+
+
+def getattr_with_deprecation(imports: Dict[str, str], module: str, name: str):
+ target_class_full_name = imports.get(name)
+ if not target_class_full_name:
+ raise AttributeError(f"The module `{module!r}` has no attribute `{name!r}`")
+ warnings.warn(
+ f"The `{module}.{name}` class is deprecated. Please use `{target_class_full_name!r}`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ new_module, new_class_name = target_class_full_name.rsplit('.', 1)
+ return getattr(importlib.import_module(new_module), new_class_name)
+
+
+def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str):
+ for submodule_name, imports in module_imports.items():
+ module_type = ModuleType(f"{module}.{submodule_name}")
Review Comment:
Not sure that it matters, but this approach does "pollute" sys.modules with all deprecated modules when any contrib module is loaded.
--
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