uranusjr commented on code in PR #26649:
URL: https://github.com/apache/airflow/pull/26649#discussion_r979486944
##########
airflow/serialization/serialized_objects.py:
##########
@@ -165,7 +165,11 @@ def __init__(self, type_string: str) -> None:
self.type_string = type_string
def __str__(self) -> str:
- return f"Timetable class {self.type_string!r} is not registered"
+ return (
+ f"Timetable class {self.type_string!r} is not registered or "
+ "you have a top level database access that disrupted the session. "
+ "Please check the airflow best practices documentation."
+ )
Review Comment:
I sort of feel that mentioning _top level database access_ is a bit too
specific. Maybe it’d be better to be slightly more vague here (there’s
something wrong with eager variable evaluation in the class) instead. But I’m
not strong on this—we can always reword this afterwards.
##########
docs/apache-airflow/concepts/timetable.rst:
##########
@@ -51,6 +51,12 @@ As such, Airflow allows for custom timetables to be written
in plugins and used
DAGs. An example demonstrating a custom timetable can be found in the
:doc:`/howto/timetable` how-to guide.
+.. note::
+
+ As a general rule, always access Variables, Connections etc or anything
that would access
+ the database as late as possible in your code. See
:ref:`best_practices/top_level_code`
+ for more best practices to follow.
Review Comment:
We should add an example here for timetable specifically here. For users not
very familiar with Python, there’s nothing in the referenced section that would
jump out and indicate what’s wrong.
(Alternatively, we can add an example in the referenced section explaining
that function argument defaults and class-level variables are also top level
code.)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
