Fix plugin macros not being exposed through airflow.macros

[RikHeijdens]

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 for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

[boring-cyborg]

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 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 Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
  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

[RikHeijdens]

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?

[mik-laj]

Will importlib.reload help?

[RikHeijdens]

Simply reloading the module using importlib.reload() will not be sufficient as the module's symbol table (dictionary) is retained.

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.

[RikHeijdens]

I've implemented this in c2b5354 -- I'm curious to your thoughts.

[mik-laj]

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?

[RikHeijdens]

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.

[github-actions]

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.

[mik-laj]

@RikHeijdens Can you do a rebase? I want to make sure that importlib tricks work on all versions of Python.

[RikHeijdens]

@mik-laj I've just rebased my branch on master.

[RikHeijdens]

@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 RikHeijdens@b2c035c):

# 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.

[mik-laj]

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.

airflow/airflow/operators/python.py

Lines 536 to 537 in 39ea872

	def _is_airflow_env():
	return self.system_site_packages or 'apache-airflow' in self.requirements

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.

[RikHeijdens]

@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.

[mik-laj]

@RikHeijdens Got it. Thanks for your work.

[ashb]

@RikHeijdens Can you document this py2 virtualenv limiation somewhere please? (VirtualEnvOperator docs?) LGTM after that.

[RikHeijdens]

I've documented the limitation in d17e98f, but I'm a bit skeptical as to whether any of the AIRFLOW_SERIALIZABLE_CONTEXT_KEYS 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.

[github-actions]

The Workflow run 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*.

[RikHeijdens]

@mik-laj It looks like the CI Build for d17e98f 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?

[mik-laj]

@RikHeijdens Yes. Please, do it. That should fix the problem.

[ashb]

Almost there. It almost feels like someone is running a slowloris attack against GitHub actions today.

[ashb]

✔️

[boring-cyborg]

Awesome work, congrats on your first merged pull request!