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 2020/12/03 17:41:44 UTC
[GitHub] [airflow] RikHeijdens opened a new pull request #12788: Fix plugin macros not being exposed through airflow.macros
RikHeijdens opened a new pull request #12788:
URL: https://github.com/apache/airflow/pull/12788
This PR fixes an issue where macros that are being provided through plugins can not be used at template time because they are not accessible through the `airflow.macros` module.
This PR consists out of two commits. In the first commit I add a test-case that reproduces the issue as outlined in #12785, and the second commit introduces the fix which fixes the issue and allows the test-case to pass.
Fixes: #12785
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535447889
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
`integrate_macros_plugins()` has the side-effect that it modifies the `airflow.macros` module. Do we need to add additional cleanup logic to reset the contents of `airflow.macros` through a finalizer, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-740204709
Almost there. It almost feels like someone is running a slowloris attack against GitHub actions today.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r537357119
##########
File path: airflow/utils/python_virtualenv_script.jinja2
##########
@@ -20,6 +20,18 @@
import {{ pickling_library }}
import sys
+# Check whether Airflow is available in the environment.
+# If it is, we'll want to ensure that we integrate any macros that are being provided
+# by plugins prior to unpickling the task context.
+if int(sys.version[0]) >= 3:
Review comment:
I'll make the check a bit more strict, just in case :-)
Edit: This has been addressed in https://github.com/apache/airflow/pull/12788/commits/cee7686ab15b5acdc4967ed775b00af48cd5990c.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535530351
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
I love this change.😻 I'm just wondering if it's worth moving this code to the context manager mock_plugins_manager. This allows us to limit the side effects in other texts as well. What do you think?
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
I love this change.😻 I'm just wondering if it's worth moving this code to the context manager mock_plugins_manager. This allows us to limit the side effects in other tests as well. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-738170463
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r537341308
##########
File path: airflow/utils/python_virtualenv_script.jinja2
##########
@@ -20,6 +20,18 @@
import {{ pickling_library }}
import sys
+# Check whether Airflow is available in the environment.
+# If it is, we'll want to ensure that we integrate any macros that are being provided
+# by plugins prior to unpickling the task context.
+if int(sys.version[0]) >= 3:
Review comment:
Airflow 2.0 only supports Python 3.6 and up. https://github.com/apache/airflow#requirements
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-739885146
[The Workflow run](https://github.com/apache/airflow/actions/runs/405739020) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-739783685
@mik-laj Got it. I've restored access to `airflow.macros` for subprocesses. However, in order to get the tests to pass I did have to implement an additional safe guard that would prevent `airflow.plugins_manager.integrate_macros_plugins` from being invoked on virtual environments that run Python 2. I don't see how this feature can be supported for any Py2 virtual environments any longer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-739796402
@RikHeijdens Can you document this py2 virtualenv limiation somewhere please? (VirtualEnvOperator docs?) LGTM after that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-738277515
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-739956273
@mik-laj It looks like the CI Build for https://github.com/apache/airflow/pull/12788/commits/d17e98f5b6e59aac572e9bfcf3889dd39a96ae2f failed due to an issue that appears to be unrelated to my proposed changes. Should I rebase my commits on master again in order to trigger a new build?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-740221554
:heavy_check_mark:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535466195
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
Will [``importlib.reload``](https://docs.python.org/3/library/importlib.html#importlib.reload) help?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-740221881
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-739792988
@RikHeijdens Got it. Thanks for your work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #12788:
URL: https://github.com/apache/airflow/pull/12788
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535447889
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
`integrate_macros_plugins()` has the side-effect that it modifies the `airflow.macros` module. Do we need to add additional cleanup logic to reset the contents of `airflow.macros` through a finalizer 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-738639805
@mik-laj I've just rebased my branch on master.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-738776508
@mik-laj I noticed the change proposed here causes the `TestPythonVirtualenvOperator.test_airflow_context` test to fail. I believe this is happening because `airflow.macros` is being serialized as part of the context which is passed to the underlying subprocess.
The subprocess in which the Python callable runs however does not integrate the macros provided by plugins. I was able to get this test to pass locally by adding the following two lines of code to `airflow/utils/python_virtualenv_script.jinja2` (see https://github.com/RikHeijdens/airflow/commit/b2c035c87f9cfa95f6185b55c74a4b3d38d9fb63):
```python
# Ensure airflow.macros has been loaded properly.
import airflow.plugins_manager
airflow.plugins_manager.integrate_macros_plugins()
```
Doing this however, seems like a bit of a nasty solution to me and also causes additional test failures. I'm curious whether you have any thoughts on what would be a better solution? What is the use-case of allowing subprocess spawned by PythonVirtualenvOperator to access plugin-provided macros?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535512072
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
Simply reloading the module using `importlib.reload()` will not be sufficient as the module's symbol table (dictionary) is [retained](https://docs.python.org/3/library/importlib.html#importlib.reload).
> When a module is reloaded, its dictionary (containing the module’s global variables) is retained. Redefinitions of names will override the old definitions, so this is generally not a problem. If the new version of a module does not define a name that was defined by the old version, the old definition remains.
I think we'll have to delete the module from `sys.modules` and import it again in order to properly remove the entries that are being added by this test case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-739964850
@RikHeijdens Yes. Please, do it. That should fix the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535525847
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
I've implemented this in https://github.com/apache/airflow/pull/12788/commits/c2b5354addb6051f689061eeb9d7ea4d6d8965e3 -- I'm curious to your thoughts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535517135
##########
File path: airflow/plugins_manager.py
##########
@@ -419,4 +420,9 @@ def integrate_macros_plugins() -> None:
if macros_module:
macros_modules.append(macros_module)
- sys.modules[macros_module.__name__] = macros_module # pylint: disable=no-member
+ # pylint: disable=no-member
+ sys.modules[macros_module.__name__] = macros_module
+ # Register the newly created module on airflow.macros such that it
+ # can be accessed when rendering templates.
+ setattr(macros, macros_module.__name__.split('.')[-1], macros_module)
Review comment:
Fixed: https://github.com/apache/airflow/pull/12788/commits/84dc19e62f31ae1722005793029df53738f03eed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535540115
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
I considered doing this, but the `__doc__` comment on the `mock_plugin_manager` seems to suggest that the scope of that mock is limited to the `airflow.plugins` module.
While that mock does in fact clear out the `macros_modules` variable in `airflow.plugins`, it does not actually attempt to reverse any (side) effects that are caused by invoking `integrate_macros_plugins()`. I did a bit of searching through the code base, and it doesn't really appear that there is any test coverage for this function beyond the test I've just added.
Because I want to avoid scope creep for the `mock_plugin_manager` fixture, and this is the only test case that actually appears to have to deal with side effects cause by calling `integrate_macros_plugins()`, I'm a bit hesitant to make changes beyond what's being proposed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535530351
##########
File path: tests/plugins/test_plugins_manager.py
##########
@@ -213,6 +214,34 @@ def test_entrypoint_plugin_errors_dont_raise_exceptions(self, caplog):
assert "Failed to import plugin test-entrypoint" in received_logs
assert ("test.plugins.test_plugins_manager", "my_fake_module not found") in import_errors.items()
+ def test_registering_plugin_macros(self):
+ """
+ Tests whether macros that originate from plugins are being registered correctly.
+ """
+ from airflow import macros
+ from airflow.plugins_manager import integrate_macros_plugins
+
+ def custom_macro():
+ return 'foo'
+
+ class MacroPlugin(AirflowPlugin):
+ name = 'macro_plugin'
+ macros = [custom_macro]
+
+ with mock_plugin_manager(plugins=[MacroPlugin()]):
+ # Ensure the macros for the plugin have been integrated.
+ integrate_macros_plugins()
+ # Test whether the modules have been created as expected.
+ plugin_macros = importlib.import_module(f"airflow.macros.{MacroPlugin.name}")
+ for macro in MacroPlugin.macros:
+ # Verify that the macros added by the plugin are being set correctly
+ # on the plugin's macro module.
+ assert hasattr(plugin_macros, macro.__name__)
+ # Verify that the symbol table in airflow.macros has been updated with an entry for
+ # this plugin, this is necessary in order to allow the plugin's macros to be used when
+ # rendering templates.
+ assert hasattr(macros, MacroPlugin.name)
Review comment:
I like this change. I'm just wondering if it's worth moving this code to the context manager mock_plugins_manager. This allows us to limit the side effects in other texts as well. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-738300903
@RikHeijdens Can you do a rebase? I want to make sure that `importlib` tricks work on all versions of Python.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens edited a comment on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens edited a comment on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-738776508
@mik-laj I noticed the change proposed here causes the `TestPythonVirtualenvOperator.test_airflow_context` test to fail. I believe this is happening because `airflow.macros` is being serialized as part of the context which is passed to the underlying subprocess.
The subprocess in which the Python callable runs however does not integrate the macros provided by plugins. I was able to get this test to pass locally by adding the following two lines of code to `airflow/utils/python_virtualenv_script.jinja2` (see https://github.com/RikHeijdens/airflow/commit/b2c035c87f9cfa95f6185b55c74a4b3d38d9fb63):
```python
# Ensure airflow.macros has been loaded properly.
import airflow.plugins_manager
airflow.plugins_manager.integrate_macros_plugins()
```
Doing this however, seems like a bit of a nasty solution to me and also causes additional test failures. I'm curious whether you have any thoughts on what would be a better solution? What is the use-case of allowing subprocess spawned by PythonVirtualenvOperator to access plugin-provided macros?
Edit: I went ahead and removed `airflow.macros` from the set of `AIRFLOW_SERIALIZABLE_CONTEXT_KEYS` in PythonVirtualEnvironmentOperator. I'm not sure whether this can be considered as a backwards-incompatible change given that this functionality is currently broken, but hopefully this should allow all tests to pass at the very least.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-738864974
> Doing this however, seems like a bit of a nasty solution to me and also causes additional test failures
I guess this code should only be called if you have airflow installed in your environment.
https://github.com/apache/airflow/blob/39ea8722c04fb1c0b286b4248a52e8d974a47b30/airflow/operators/python.py#L536-L537
The use case is that we want to allow the user to perform any operation, but in a virtual environment. The virtual environment will allow they to use additional libraries.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r537357119
##########
File path: airflow/utils/python_virtualenv_script.jinja2
##########
@@ -20,6 +20,18 @@
import {{ pickling_library }}
import sys
+# Check whether Airflow is available in the environment.
+# If it is, we'll want to ensure that we integrate any macros that are being provided
+# by plugins prior to unpickling the task context.
+if int(sys.version[0]) >= 3:
Review comment:
I'll make the check a bit more strict, just in case :-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RikHeijdens commented on pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
RikHeijdens commented on pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#issuecomment-739801030
I've documented the limitation in https://github.com/apache/airflow/pull/12788/commits/d17e98f5b6e59aac572e9bfcf3889dd39a96ae2f, but I'm a bit skeptical as to whether any of the [`AIRFLOW_SERIALIZABLE_CONTEXT_KEYS`](https://github.com/apache/airflow/blob/master/airflow/operators/python.py#L434) can be used in a virtual environment that runs Python 2.
I believe anything that will require to import `airflow.*` no longer works, but there is also no test-coverage that confirms this. I also want to prevent any additional scope-creep with this PR, so I think it may be better to make a new issue 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12788: Fix plugin macros not being exposed through airflow.macros
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12788:
URL: https://github.com/apache/airflow/pull/12788#discussion_r535461911
##########
File path: airflow/plugins_manager.py
##########
@@ -419,4 +420,9 @@ def integrate_macros_plugins() -> None:
if macros_module:
macros_modules.append(macros_module)
- sys.modules[macros_module.__name__] = macros_module # pylint: disable=no-member
+ # pylint: disable=no-member
+ sys.modules[macros_module.__name__] = macros_module
+ # Register the newly created module on airflow.macros such that it
+ # can be accessed when rendering templates.
+ setattr(macros, macros_module.__name__.split('.')[-1], macros_module)
Review comment:
```suggestion
setattr(macros, plugin.name, macros_module)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org