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/11/19 02:25:28 UTC
[GitHub] [airflow] potiuk opened a new pull request #12472: Add extra links to providers
potiuk opened a new pull request #12472:
URL: https://github.com/apache/airflow/pull/12472
Based on #12465
Closes: #11431
<!--
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 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/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] mik-laj commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532049617
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+BUILTIN_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
Shouldn't we cache ProviderManager instances? It seems to me that creating this object is the most expensive, but not reading the extra_links_class_names attribute?
----------------------------------------------------------------
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 a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528740348
##########
File path: scripts/in_container/run_install_and_test_provider_packages.sh
##########
@@ -93,6 +93,27 @@ function discover_all_provider_packages() {
fi
}
+function discover_all_extra_links() {
+ echo
+ echo Listing available extra links via 'airflow providers extra_links'
+ echo
+
+ airflow providers extra_links
+
+ local expected_number_of_extra_links=4
+ local actual_number_of_extra_links
+ actual_number_of_extra_links=$(airflow providers extra_links --output simple | grep -c "airflow.providers" | xargs)
+ if [[ ${actual_number_of_extra_links} != "${expected_number_of_extra_links}" ]]; then
+ >&2 echo "ERROR! Number of extra links installed is wrong!"
+ >&2 echo "Expected number was '${expected_number_of_extra_links}' and got '${actual_number_of_extra_links}'"
+ >&2 echo
+ >&2 echo "Either increase the number of links if you added one or fix the problem with imports if you see one."
+ >&2 echo
+ fi
+}
Review comment:
I'd prefer it if this test could be in python, not bash -- if we have an easy place to put it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532058523
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+BUILTIN_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
+def get_operator_extra_links():
+ """
+ Returns operator extra links - both the ones that are built in and the ones that come from
+ the providers.
+ :return: set of extra links
+ """
+ BUILTIN_OPERATOR_EXTRA_LINKS.update(ProvidersManager().extra_links_class_names)
Review comment:
Yep. I thought I fixed 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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533839528
##########
File path: airflow/providers_manager.py
##########
@@ -68,6 +68,7 @@ def __init__(self):
# Keeps dict of hooks keyed by connection type and value is
# Tuple: connection class, connection_id_attribute_name
self._hooks_dict: Dict[str, Tuple[str, str]] = {}
+ self.__extra_link_class_name_set: Set[str] = set()
Review comment:
of course.
----------------------------------------------------------------
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] potiuk commented on pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-734986326
Looks like good to go!
----------------------------------------------------------------
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] potiuk commented on pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-736934261
Shoudl be ready to merge everyone -> all comments applied. I hope it gets green.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r534073102
##########
File path: airflow/providers_manager.py
##########
@@ -224,6 +226,43 @@ def _add_hook(self, hook_class_name, provider_package) -> None:
self._hooks_dict[conn_type] = (hook_class_name, connection_id_attribute_name)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
+ """
+ Adds extra link class name to the list of classes
+ :param extra_link_class_name: name of the class to add
+ :param provider_package: provider package adding the link
+ :return:
+ """
+ if provider_package.startswith("apache-airflow"):
+ provider_path = provider_package[len("apache-") :].replace("-", ".")
+ if not extra_link_class_name.startswith(provider_path):
+ log.warning(
+ "Sanity check failed when importing '%s' from '%s' package. It should start with '%s'",
+ extra_link_class_name,
+ provider_package,
+ provider_path,
+ )
+ return
+ try:
+ module, class_name = extra_link_class_name.rsplit('.', maxsplit=1)
+ getattr(importlib.import_module(module), class_name)
Review comment:
I'm just thinking of @mik-laj's comment on another PR (I forget exactly which one, sorry) where he said importing all of the GCP providers would use a lot of memory.
Cool -- removing it here would be good, then we only pay the price when someone actually uses one of those operators, rather just by having the provider installed (but never using the specific operator that has the link)
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532099859
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
oh but the file just contains 1 import currently so didn't know:
```
from functools import lru_cache
```
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r535358117
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
Added the conditional import. Should be good to go.
----------------------------------------------------------------
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 #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-736983895
[The Workflow run](https://github.com/apache/airflow/actions/runs/395332149) 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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532291430
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
Will add some comments if I decide to leave the cache . Not sure yet :)
----------------------------------------------------------------
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] potiuk merged pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #12472:
URL: https://github.com/apache/airflow/pull/12472
----------------------------------------------------------------
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 #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-730103503
[The Workflow run](https://github.com/apache/airflow/actions/runs/371491579) 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] github-actions[bot] commented on pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-738247066
[The Workflow run](https://github.com/apache/airflow/actions/runs/399104204) 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] github-actions[bot] commented on pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-736983954
[The Workflow run](https://github.com/apache/airflow/actions/runs/395332741) 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] ashb commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532099216
##########
File path: airflow/providers_manager.py
##########
@@ -178,7 +181,49 @@ def _add_provider_info_from_local_source_file(self, path, package_name) -> None:
except Exception as e: # noqa pylint: disable=broad-except
log.warning("Error when loading '%s': %s", path, e)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
Review comment:
This makes it "private" -- python in the hood translates this to `__ProvidersManager_add_extra_link`
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532099960
##########
File path: airflow/providers_manager.py
##########
@@ -178,7 +181,49 @@ def _add_provider_info_from_local_source_file(self, path, package_name) -> None:
except Exception as e: # noqa pylint: disable=broad-except
log.warning("Error when loading '%s': %s", path, e)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
Review comment:
Yup, just wanted to confirm though if that was intentional
----------------------------------------------------------------
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 #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532083991
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+OPERATOR_EXTRA_LINKS: Set[str] = {
Review comment:
```suggestion
_OPERATOR_EXTRA_LINKS: Set[str] = {
```
I don't want anyone to accidentally use this variable instead of calling functions `get_operator_extra_links`.
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533841635
##########
File path: airflow/providers_manager.py
##########
@@ -224,6 +226,43 @@ def _add_hook(self, hook_class_name, provider_package) -> None:
self._hooks_dict[conn_type] = (hook_class_name, connection_id_attribute_name)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
+ """
+ Adds extra link class name to the list of classes
+ :param extra_link_class_name: name of the class to add
+ :param provider_package: provider package adding the link
+ :return:
+ """
+ if provider_package.startswith("apache-airflow"):
+ provider_path = provider_package[len("apache-") :].replace("-", ".")
+ if not extra_link_class_name.startswith(provider_path):
+ log.warning(
+ "Sanity check failed when importing '%s' from '%s' package. It should start with '%s'",
+ extra_link_class_name,
+ provider_package,
+ provider_path,
+ )
+ return
+ try:
+ module, class_name = extra_link_class_name.rsplit('.', maxsplit=1)
+ getattr(importlib.import_module(module), class_name)
Review comment:
Surely, I wanted to get more of a sanity check and "early" warning in the logs of Airflow. The way it is done right now is that the class will be anyhow imported in the "deserialize_operator" method, which I think is 'good enough" so I am happy to remove it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532073830
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+BUILTIN_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
Ah no. It is ok here. I want to still get it from the cache so i do not update the dict all the time only the first time. it's really small optimisation, but I just wanted to make sure it is only set once. Otherwise I'd have to re-create the dictionary every time (and as mentioned above ProvidersManager is a singleton, so no need to cache anything there.).
----------------------------------------------------------------
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 #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r534815915
##########
File path: airflow/providers_manager.py
##########
@@ -224,6 +226,43 @@ def _add_hook(self, hook_class_name, provider_package) -> None:
self._hooks_dict[conn_type] = (hook_class_name, connection_id_attribute_name)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
+ """
+ Adds extra link class name to the list of classes
+ :param extra_link_class_name: name of the class to add
+ :param provider_package: provider package adding the link
+ :return:
+ """
+ if provider_package.startswith("apache-airflow"):
+ provider_path = provider_package[len("apache-") :].replace("-", ".")
+ if not extra_link_class_name.startswith(provider_path):
+ log.warning(
+ "Sanity check failed when importing '%s' from '%s' package. It should start with '%s'",
+ extra_link_class_name,
+ provider_package,
+ provider_path,
+ )
+ return
+ try:
+ module, class_name = extra_link_class_name.rsplit('.', maxsplit=1)
+ getattr(importlib.import_module(module), class_name)
Review comment:
In the past, we loaded all extra links during Airflow startup. See: https://github.com/apache/airflow/pull/7327
----------------------------------------------------------------
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 a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r535366086
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -20,17 +20,25 @@
import enum
import logging
from inspect import Parameter, signature
-from typing import Any, Dict, Iterable, List, Optional, Set, Union
+from typing import Any, Dict, Iterable, Optional, Set, Union
import cattr
import pendulum
from dateutil import relativedelta
+
+try:
+ from functools import cache
+except ImportError:
+ from functools import lru_cache
+
+ cache = lru_cache(maxsize=None)
Review comment:
(Style okay with this? I would have expected a new line or two after 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] ashb commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533728794
##########
File path: airflow/provider.yaml.schema.json
##########
@@ -180,6 +180,13 @@
"items": {
"type": "string"
}
+ },
+ "extra-links": {
+ "type": "array",
+ "description": "Class nme that provide extra link functionality",
Review comment:
```suggestion
"description": "Class name that provide extra link functionality",
```
----------------------------------------------------------------
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 #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-736983441
[The Workflow run](https://github.com/apache/airflow/actions/runs/395330878) 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] kaxil commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532098091
##########
File path: airflow/providers_manager.py
##########
@@ -178,7 +181,49 @@ def _add_provider_info_from_local_source_file(self, path, package_name) -> None:
except Exception as e: # noqa pylint: disable=broad-except
log.warning("Error when loading '%s': %s", path, e)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
Review comment:
Just want to double check`__` is intentional to assign it to class right?
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528901385
##########
File path: scripts/in_container/run_install_and_test_provider_packages.sh
##########
@@ -93,6 +93,27 @@ function discover_all_provider_packages() {
fi
}
+function discover_all_extra_links() {
+ echo
+ echo Listing available extra links via 'airflow providers extra_links'
+ echo
+
+ airflow providers extra_links
+
+ local expected_number_of_extra_links=4
+ local actual_number_of_extra_links
+ actual_number_of_extra_links=$(airflow providers extra_links --output simple | grep -c "airflow.providers" | xargs)
+ if [[ ${actual_number_of_extra_links} != "${expected_number_of_extra_links}" ]]; then
+ >&2 echo "ERROR! Number of extra links installed is wrong!"
+ >&2 echo "Expected number was '${expected_number_of_extra_links}' and got '${actual_number_of_extra_links}'"
+ >&2 echo
+ >&2 echo "Either increase the number of links if you added one or fix the problem with imports if you see one."
+ >&2 echo
+ fi
+}
Review comment:
This is quite a different test - this is an integration test.
The Python tests by definition use sources, So they are loading provider.yaml - almost by definition the unit tests cannot really rely on packages being installed and the package discovery mechanism.
That's why it is in bash. It performs end-to-end process: builds airflow .whl, installs airflow from .whl. builds all providers from .whl (or .tar.gz) and finally it checks if those providers are all properly discovered.
For example in this case it will also detect any problems coming from contextual version conflicts. Not all operators will be registered during this test if some of them fails to run entry_point because of the Version Conflict.
For now this comparison does not result in error. because we actually have some version conflicts to resolve.
IMHO it SHOULD be in bash because of the nature of it. It just simulates real users installing airflow and running the CLI..
----------------------------------------------------------------
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 a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r534073873
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
Can you add a comment since we're keeping the cache?
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532291263
##########
File path: airflow/providers_manager.py
##########
@@ -178,7 +181,49 @@ def _add_provider_info_from_local_source_file(self, path, package_name) -> None:
except Exception as e: # noqa pylint: disable=broad-except
log.warning("Error when loading '%s': %s", path, e)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
Review comment:
Yep. And on Ash's request I am changing it to protected.
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528901385
##########
File path: scripts/in_container/run_install_and_test_provider_packages.sh
##########
@@ -93,6 +93,27 @@ function discover_all_provider_packages() {
fi
}
+function discover_all_extra_links() {
+ echo
+ echo Listing available extra links via 'airflow providers extra_links'
+ echo
+
+ airflow providers extra_links
+
+ local expected_number_of_extra_links=4
+ local actual_number_of_extra_links
+ actual_number_of_extra_links=$(airflow providers extra_links --output simple | grep -c "airflow.providers" | xargs)
+ if [[ ${actual_number_of_extra_links} != "${expected_number_of_extra_links}" ]]; then
+ >&2 echo "ERROR! Number of extra links installed is wrong!"
+ >&2 echo "Expected number was '${expected_number_of_extra_links}' and got '${actual_number_of_extra_links}'"
+ >&2 echo
+ >&2 echo "Either increase the number of links if you added one or fix the problem with imports if you see one."
+ >&2 echo
+ fi
+}
Review comment:
This is quite a different test - this is an integration test.
The Python tests by definition use sources, So they are loading provider.yaml - almost by definition the unit tests cannot really rely on packages being installed and the package discovery mechanism.
That's why it is in bash. It performs end-to-end process: builds airflow .whl, installs airflow from .whl. builds all providers from .whl (or .tar.gz) installs them, and finally - it checks if those providers are all properly discovered.
For example in this case it will also detect any problems coming from contextual version conflicts. Not all operators will be registered during this test if some of them fails to run entry_point because of the Version Conflict.
For now this comparison does not result in error. because we actually have some version conflicts to resolve.
IMHO it SHOULD be in bash because of the nature of it. It just simulates real users installing airflow and running the CLI..
----------------------------------------------------------------
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 a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528741097
##########
File path: scripts/in_container/run_install_and_test_provider_packages.sh
##########
@@ -93,6 +93,27 @@ function discover_all_provider_packages() {
fi
}
+function discover_all_extra_links() {
+ echo
+ echo Listing available extra links via 'airflow providers extra_links'
+ echo
+
+ airflow providers extra_links
+
+ local expected_number_of_extra_links=4
+ local actual_number_of_extra_links
+ actual_number_of_extra_links=$(airflow providers extra_links --output simple | grep -c "airflow.providers" | xargs)
+ if [[ ${actual_number_of_extra_links} != "${expected_number_of_extra_links}" ]]; then
+ >&2 echo "ERROR! Number of extra links installed is wrong!"
+ >&2 echo "Expected number was '${expected_number_of_extra_links}' and got '${actual_number_of_extra_links}'"
+ >&2 echo
+ >&2 echo "Either increase the number of links if you added one or fix the problem with imports if you see one."
+ >&2 echo
+ fi
+}
Review comment:
Wait -- isn't this very similar to `test_extra_links` - what's the difference?
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533839041
##########
File path: airflow/cli/cli_parser.py
##########
@@ -1171,6 +1171,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command('airflow.cli.commands.provider_command.provider_get'),
args=(ARG_OUTPUT, ARG_FULL, ARG_COLOR, ARG_PROVIDER_NAME),
),
+ ActionCommand(
+ name='extra_links',
Review comment:
Right!
----------------------------------------------------------------
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 a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532099145
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
This was because in newer versions of func_tools there is a `cache` function that does this, so it's mirroring that. Could probably do with a comment, or at the top:
```python
try:
from functools import cache
except ImportError:
from functools import lru_cache
cache = lru_cache(maxsize=None)
```
----------------------------------------------------------------
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 a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533731217
##########
File path: airflow/providers_manager.py
##########
@@ -224,6 +226,43 @@ def _add_hook(self, hook_class_name, provider_package) -> None:
self._hooks_dict[conn_type] = (hook_class_name, connection_id_attribute_name)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
+ """
+ Adds extra link class name to the list of classes
+ :param extra_link_class_name: name of the class to add
+ :param provider_package: provider package adding the link
+ :return:
+ """
+ if provider_package.startswith("apache-airflow"):
+ provider_path = provider_package[len("apache-") :].replace("-", ".")
+ if not extra_link_class_name.startswith(provider_path):
+ log.warning(
+ "Sanity check failed when importing '%s' from '%s' package. It should start with '%s'",
+ extra_link_class_name,
+ provider_package,
+ provider_path,
+ )
+ return
+ try:
+ module, class_name = extra_link_class_name.rsplit('.', maxsplit=1)
+ getattr(importlib.import_module(module), class_name)
Review comment:
Do we need actually to import all the classes here? That's potentially a lot of extra code that is imported in to the scheduler as a result, that is otherwise un-needed (if I'm reading where this is used/called from right)
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528898160
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+BUILTIN_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
+def get_builtin_operator_extra_links():
Review comment:
True
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532091587
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+OPERATOR_EXTRA_LINKS: Set[str] = {
Review comment:
Good idea.
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533839398
##########
File path: airflow/provider.yaml.schema.json
##########
@@ -180,6 +180,13 @@
"items": {
"type": "string"
}
+ },
+ "extra-links": {
+ "type": "array",
+ "description": "Class nme that provide extra link functionality",
Review comment:
Yep.
----------------------------------------------------------------
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 #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-736984146
[The Workflow run](https://github.com/apache/airflow/actions/runs/395334613) 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] potiuk commented on pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-732335632
I think everything addressed here as well (and rebsed on top of the updated "base PR". I really find useful to run the integration tests in bash @ashb -> those are really valuable to see if airflow works E2E. Especially when we fix version conflicts it will become a useful tool to verify if the discovery works using the package mechanism.
----------------------------------------------------------------
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] potiuk commented on pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-736934075
Applied all comments @ashb
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r535375578
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -20,17 +20,25 @@
import enum
import logging
from inspect import Parameter, signature
-from typing import Any, Dict, Iterable, List, Optional, Set, Union
+from typing import Any, Dict, Iterable, Optional, Set, Union
import cattr
import pendulum
from dateutil import relativedelta
+
+try:
+ from functools import cache
+except ImportError:
+ from functools import lru_cache
+
+ cache = lru_cache(maxsize=None)
Review comment:
BTW. I stopped wasting my brain-cycles on formatting since we have black :)
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532099859
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
oh but the file just contains 1 import currently so didn't know:
```
from functools import lru_cache
```
https://github.com/apache/airflow/pull/12472/files#diff-807ca0a4fd53aeb41166621c9842b0f89b7931fc64e9a60befa36c776db45efaR22
----------------------------------------------------------------
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 #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532049784
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+BUILTIN_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
+def get_operator_extra_links():
+ """
+ Returns operator extra links - both the ones that are built in and the ones that come from
+ the providers.
+ :return: set of extra links
+ """
+ BUILTIN_OPERATOR_EXTRA_LINKS.update(ProvidersManager().extra_links_class_names)
Review comment:
This variable name does not describe the content well. These are not builtin links if they are from providers packages.
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528894334
##########
File path: airflow/providers/google/provider.yaml
##########
@@ -633,3 +633,8 @@ transfers:
- source-integration-name: Google Ads
target-integration-name: Google Cloud Storage (GCS)
python-module: airflow.providers.google.ads.transfers.ads_to_gcs
+
+extra-links:
+ - airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink
+ - airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink
+ - airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink
Review comment:
Yeh. that is alreeady settled :)
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528897422
##########
File path: airflow/provider.yaml.schema.json
##########
@@ -167,6 +167,13 @@
"python-module"
]
}
+ },
+ "extra-links": {
+ "type": "array",
Review comment:
Yeah. I had it initially, there are a few more places where we can add uniqueness/regexp rules. So maybe we should do it as a separate PR once we merge those. By that time we will have all the values in and tests to check if all our regexps/rules are valid
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532058485
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+BUILTIN_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
This cache is to be removed. I've changed ProviderManager to be singleton so it does not need caching either.
----------------------------------------------------------------
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 a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528738432
##########
File path: airflow/providers/google/provider.yaml
##########
@@ -633,3 +633,8 @@ transfers:
- source-integration-name: Google Ads
target-integration-name: Google Cloud Storage (GCS)
python-module: airflow.providers.google.ads.transfers.ads_to_gcs
+
+extra-links:
+ - airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink
+ - airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink
+ - airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink
Review comment:
Yes, I strongly prefer full paths too, so I'm glad you suggested it first. (I prefer it, but it doesn't _matter_ to me in this case so I didn't mention it)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528657993
##########
File path: airflow/provider.yaml.schema.json
##########
@@ -167,6 +167,13 @@
"python-module"
]
}
+ },
+ "extra-links": {
+ "type": "array",
Review comment:
We can define a (basic) pattern at the schema level too:
```suggestion
"type": "array",
"uniqueItems": true,
"pattern": "^\.[a-zA-Z]+[a-zA-Z0-9.\\-]*"
```
##########
File path: airflow/providers/google/provider.yaml
##########
@@ -633,3 +633,8 @@ transfers:
- source-integration-name: Google Ads
target-integration-name: Google Cloud Storage (GCS)
python-module: airflow.providers.google.ads.transfers.ads_to_gcs
+
+extra-links:
+ - airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink
+ - airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink
+ - airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink
Review comment:
Aren't these invalid according to the schema:
```suggestion
- .operators.bigquery.BigQueryConsoleLink
- .operators.bigquery.BigQueryConsoleIndexableLink
- .operators.mlengine.AIPlatformConsoleLink
```
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528901385
##########
File path: scripts/in_container/run_install_and_test_provider_packages.sh
##########
@@ -93,6 +93,27 @@ function discover_all_provider_packages() {
fi
}
+function discover_all_extra_links() {
+ echo
+ echo Listing available extra links via 'airflow providers extra_links'
+ echo
+
+ airflow providers extra_links
+
+ local expected_number_of_extra_links=4
+ local actual_number_of_extra_links
+ actual_number_of_extra_links=$(airflow providers extra_links --output simple | grep -c "airflow.providers" | xargs)
+ if [[ ${actual_number_of_extra_links} != "${expected_number_of_extra_links}" ]]; then
+ >&2 echo "ERROR! Number of extra links installed is wrong!"
+ >&2 echo "Expected number was '${expected_number_of_extra_links}' and got '${actual_number_of_extra_links}'"
+ >&2 echo
+ >&2 echo "Either increase the number of links if you added one or fix the problem with imports if you see one."
+ >&2 echo
+ fi
+}
Review comment:
This is quite a different test - this is an integration test.
That's why it is in bash. It performs end-to-end process: builds airflow .whl, installs airflow from .whl. builds all providers from .whl (or .tar.gz) and finally it checks if those providers are all properly discovered.
For example in this case it will also detect any problems coming from contextual version conflicts. Not all operators will be registered during this test if some of them fails to run entry_point because of the Version Conflict.
For now this comparison does not result in error. because we actually have some version conflicts to resolve.
IMHO it SHOULD be in bash because of the nature of it. It just simulates real users installing airflow and running the CLI..
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528764576
##########
File path: airflow/providers_manager.py
##########
@@ -62,6 +64,7 @@ def __init__(self):
self.__find_all_airflow_builtin_providers_from_local_sources()
self.__find_all_providers_from_packages()
self.__creates_provider_directory()
+ self.__retrieve_extra_links()
Review comment:
Discover is a good name.
----------------------------------------------------------------
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 a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r534073873
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
Can you add a comment since we're keeping the cache?
Just this then LGTM @potiuk
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532098341
##########
File path: airflow/providers_manager.py
##########
@@ -178,7 +181,49 @@ def _add_provider_info_from_local_source_file(self, path, package_name) -> None:
except Exception as e: # noqa pylint: disable=broad-except
log.warning("Error when loading '%s': %s", path, e)
+ def _discover_extra_links(self) -> None:
+ """Retrieves all extra links defined in the providers"""
+ for provider_package, (_, provider) in self._provider_dict.items():
+ if provider.get("extra-links"):
+ for extra_link in provider["extra-links"]:
+ self.__add_extra_link(extra_link, provider_package)
+
+ def __add_extra_link(self, extra_link_class_name, provider_package) -> None:
+ """
+ Adds extra link class name to the list of classes
+ :param extra_link_class_name: name of the class to add
+ :param provider_package: provider package adding the link
+ :return:
+ """
+ if provider_package.startswith("apache-airflow"):
+ provider_path = provider_package[len("apache-") :].replace("-", ".")
+ if not extra_link_class_name.startswith(provider_path):
+ log.warning(
+ "Sanity check failed when importing '%s' from '%s' package. It should start with '%s'",
+ extra_link_class_name,
+ provider_package,
+ provider_path,
+ )
+ return
+ try:
+ module, class_name = extra_link_class_name.rsplit('.', maxsplit=1)
+ getattr(importlib.import_module(module), class_name)
+ except Exception as e: # noqa pylint: disable=broad-except
+ log.warning(
+ "The '%s' extra-link from '%s' could not be imported: %s",
+ extra_link_class_name,
+ provider_package,
+ e,
+ )
+ return
+ self.__extra_link_class_name_set.add(extra_link_class_name)
+
@property
def providers(self):
"""Returns information about available providers."""
return self._provider_dict
+
+ @property
+ def extra_links_class_names(self):
+ """Returns set of extra link class names."""
+ return sorted(list(self.__extra_link_class_name_set))
Review comment:
```suggestion
return sorted(self.__extra_link_class_name_set)
```
sorted would return a list anyway:
```python
In [1]: a = {'d', 'a', 'c'}
In [2]: sorted(a)
Out[2]: ['a', 'c', 'd']
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533729098
##########
File path: airflow/providers_manager.py
##########
@@ -68,6 +68,7 @@ def __init__(self):
# Keeps dict of hooks keyed by connection type and value is
# Tuple: connection class, connection_id_attribute_name
self._hooks_dict: Dict[str, Tuple[str, str]] = {}
+ self.__extra_link_class_name_set: Set[str] = set()
Review comment:
```suggestion
self._extra_link_class_name_set: Set[str] = set()
```
for consistency?
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r535374856
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -20,17 +20,25 @@
import enum
import logging
from inspect import Parameter, signature
-from typing import Any, Dict, Iterable, List, Optional, Set, Union
+from typing import Any, Dict, Iterable, Optional, Set, Union
import cattr
import pendulum
from dateutil import relativedelta
+
+try:
+ from functools import cache
+except ImportError:
+ from functools import lru_cache
+
+ cache = lru_cache(maxsize=None)
Review comment:
It was black-formatted, I stopped caring since we have pre-commit enabled. If black-thinks it is, good, it must be good.
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533838822
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
+def get_operator_extra_links():
+ """
+ Returns operator extra links - both the ones that are built in and the ones that come from
+ the providers.
+ :return: set of extra links
Review comment:
Right
----------------------------------------------------------------
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 #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-738123311
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] github-actions[bot] commented on pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-738986102
[The Workflow run](https://github.com/apache/airflow/actions/runs/401290825) 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] ashb commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r533728684
##########
File path: airflow/cli/cli_parser.py
##########
@@ -1171,6 +1171,12 @@ class GroupCommand(NamedTuple):
func=lazy_load_command('airflow.cli.commands.provider_command.provider_get'),
args=(ARG_OUTPUT, ARG_FULL, ARG_COLOR, ARG_PROVIDER_NAME),
),
+ ActionCommand(
+ name='extra_links',
Review comment:
```suggestion
name='extra-links',
```
Is the new style.
----------------------------------------------------------------
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 #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528731278
##########
File path: airflow/providers/google/provider.yaml
##########
@@ -633,3 +633,8 @@ transfers:
- source-integration-name: Google Ads
target-integration-name: Google Cloud Storage (GCS)
python-module: airflow.providers.google.ads.transfers.ads_to_gcs
+
+extra-links:
+ - airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink
+ - airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink
+ - airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink
Review comment:
I prefer to use full import paths for several reasons:
1) a) when you want to use it, you can easily do it without transforming these values.
```
value = "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink"
importlib.import_module(value)
```
2) it is easier to find all references to a given class if we use the full path. I often right-click on a class and copy the reference so that I can search for it in the code. This way I can easily find class references that are not in the code, e.g. in mocks or documentation.
<img width="738" alt="Screenshot 2020-11-23 at 15 08 01" src="https://user-images.githubusercontent.com/12058428/99971605-b4b7ec80-2d9d-11eb-965b-2a48e36ffee1.png">
<img width="845" alt="Screenshot 2020-11-23 at 15 09 52" src="https://user-images.githubusercontent.com/12058428/99971794-f6489780-2d9d-11eb-8025-e4a089a35273.png">
3) We don't commonly use relative path imports in a project, so this convention may not be obvious to a new contributor. The full path is easier for new people to understand.
It is no problem to perform the necessary validation in the code and display a readable error message when there is a problem. A readable message will be friendlier than a less common convention.
What do you think about it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528739613
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+BUILTIN_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
+def get_builtin_operator_extra_links():
Review comment:
This function will no longer just return build in ones:
```suggestion
def get_registered_operator_extra_links():
```
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+BUILTIN_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
+def get_builtin_operator_extra_links():
Review comment:
This function will no longer just return build in ones:
```suggestion
def get_registered_operator_extra_links():
```
----------------------------------------------------------------
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 a change in pull request #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r528743279
##########
File path: airflow/providers_manager.py
##########
@@ -62,6 +64,7 @@ def __init__(self):
self.__find_all_airflow_builtin_providers_from_local_sources()
self.__find_all_providers_from_packages()
self.__creates_provider_directory()
+ self.__retrieve_extra_links()
Review comment:
Different verb used here (`find` vs `retrieve`) I think we should be consistent. Perhaps renaming all of them to `discover_*`?
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12472: Allow registering extra links for providers
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#discussion_r532098452
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
Review comment:
```suggestion
@lru_cache(maxsize=None)
```
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -53,14 +55,23 @@
log = logging.getLogger(__name__)
FAILED = 'serialization_failed'
-BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink",
- "airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink",
- "airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink",
- "airflow.providers.qubole.operators.qubole.QDSLink",
+_OPERATOR_EXTRA_LINKS: Set[str] = {
"airflow.operators.dagrun_operator.TriggerDagRunLink",
"airflow.sensors.external_task_sensor.ExternalTaskSensorLink",
-]
+}
+
+cache = lru_cache(maxsize=None)
+
+
+@cache
+def get_operator_extra_links():
+ """
+ Returns operator extra links - both the ones that are built in and the ones that come from
+ the providers.
+ :return: set of extra links
Review comment:
```suggestion
:return: set of extra links
```
----------------------------------------------------------------
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 #12472: Add extra links to providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12472:
URL: https://github.com/apache/airflow/pull/12472#issuecomment-731991963
[The Workflow run](https://github.com/apache/airflow/actions/runs/378468811) 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