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 2021/09/02 13:12:08 UTC
[GitHub] [airflow] uranusjr opened a new pull request #17989: Require timetable class be registered via plugin
uranusjr opened a new pull request #17989:
URL: https://github.com/apache/airflow/pull/17989
Close #17931.
The user will need to put custom timetables in a plugin and do
```python
class MyPlugin(AirflowPlugin):
timetables = [MyTimetable]
```
One question since I need to incorporate this into the documentation (#17552). Where should I put the example plugin code? Since timetable requires registration, the user can’t declare a timetable in a DAG file anymore; #17552 is currently putting the timetable code in `example_dags`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #17989: Require timetable class be registered via plugin
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17989:
URL: https://github.com/apache/airflow/pull/17989#issuecomment-912364581
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 main 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17989: Require timetable class be registered via plugin
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17989:
URL: https://github.com/apache/airflow/pull/17989#discussion_r701573209
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -128,22 +122,39 @@ def decode_timezone(var: Union[str, int]) -> Timezone:
return pendulum.timezone(var)
-def encode_timetable(var: Timetable) -> Dict[str, Any]:
+def _get_registered_timetable(importable_string: str) -> Optional[Type[Timetable]]:
+ from airflow import plugins_manager
+
+ if importable_string.startswith("airflow.timetables."):
+ return import_string(importable_string)
+ plugins_manager.initialize_timetables_plugins()
+ return plugins_manager.timetable_classes.get(importable_string)
+
+
+def _encode_timetable(var: Timetable) -> Dict[str, Any]:
"""Encode a timetable instance.
This delegates most of the serialization work to the type, so the behavior
can be completely controlled by a custom subclass.
"""
- return {"type": as_importable_string(type(var)), "value": var.serialize()}
+ timetable_class = type(var)
+ importable_string = as_importable_string(timetable_class)
+ if _get_registered_timetable(importable_string) != timetable_class:
+ raise AirflowException(f"Cannot encode unregistered timetable class {importable_string!r}")
Review comment:
This exception is actually never shown in `ImportError` since `SerializedDAG.serialize_dag()` swallows it and shows *Failed to serialize DAG 'dag_id'* instead. I’ve modified the message to append what caused the serialisation failure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #17989: Require timetable class be registered via plugin
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #17989:
URL: https://github.com/apache/airflow/pull/17989
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #17989: Require timetable class be registered via plugin
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17989:
URL: https://github.com/apache/airflow/pull/17989#discussion_r701078245
##########
File path: airflow/serialization/serialized_objects.py
##########
@@ -128,22 +122,39 @@ def decode_timezone(var: Union[str, int]) -> Timezone:
return pendulum.timezone(var)
-def encode_timetable(var: Timetable) -> Dict[str, Any]:
+def _get_registered_timetable(importable_string: str) -> Optional[Type[Timetable]]:
+ from airflow import plugins_manager
+
+ if importable_string.startswith("airflow.timetables."):
+ return import_string(importable_string)
+ plugins_manager.initialize_timetables_plugins()
+ return plugins_manager.timetable_classes.get(importable_string)
+
+
+def _encode_timetable(var: Timetable) -> Dict[str, Any]:
"""Encode a timetable instance.
This delegates most of the serialization work to the type, so the behavior
can be completely controlled by a custom subclass.
"""
- return {"type": as_importable_string(type(var)), "value": var.serialize()}
+ timetable_class = type(var)
+ importable_string = as_importable_string(timetable_class)
+ if _get_registered_timetable(importable_string) != timetable_class:
+ raise AirflowException(f"Cannot encode unregistered timetable class {importable_string!r}")
Review comment:
This _might_ be the error that shows up as the import error if you try to use a timetable in a dag that isn't registered, so we should probably change the error to be more helpful to users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org