Repository navigation
Conversation
FrankYang0529
left a comment
There was a problem hiding this comment.
Thanks for the PR. Leave some comments.
shahar1
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
The idea looks solid overall, the PR should be polished a bit more.
Besides, I think it's worth to post a discussion followed by lazy consensus on the dev list, since it involves migrations and schema changes (I wouldn't write a full AIP for that).
@uranusjr - you might want to take a look as well at some point.
| session.execute( | ||
| delete(DagWarning).where( | ||
| tuple_(DagWarning.bundle_name, DagWarning.filename).in_(files_parsed), | ||
| DagWarning.warning_type == PARSE_FAILURE_WARNING_TYPE, |
There was a problem hiding this comment.
-
emit_dag_warning()accepts a customwarning_type, but the cleanup functions, the access filter and the Dashboard card only look atPARSE_FAILURE_WARNING_TYPE`. -
The cleanup for inactive Dags also skips
these rows, so a warning with a custom type stays in the table forever.
| ) | ||
| return statement.where( | ||
| or_( | ||
| DagWarning.dag_id.in_(authorized), |
There was a problem hiding this comment.
Anyone who can read one Dag in the file sees the stacktrace. For import errors from the same file, the API hides
the stacktrace unless the user can read all Dags in it. If no Dag in the file loaded, nobody can see the warning.
| ) | ||
|
|
||
|
|
||
| class AddParseWarningsToDagFileParsingResult(VersionChange): |
There was a problem hiding this comment.
Could you please add a docstring? (otherwise description below is None)
| ) | ||
|
|
||
| self._process_modules(modules, result, bundle=bundle) | ||
| from airflow.sdk.definitions.parse_warning import drain_parse_warnings |
There was a problem hiding this comment.
Could you extract the import to the top-level of this module?
| ), | ||
| Index("idx_dag_warning_dag_id", dag_id), | ||
| bundle_name: Mapped[str | None] = mapped_column(StringID(), nullable=True) | ||
| filename: Mapped[str | None] = mapped_column(String(1024), nullable=True) |
There was a problem hiding this comment.
Every parse now runs an extra DELETE on (bundle_name, filename), and neither column is indexed. The PR raised the
statement counts in test_manager.py to fit it.
| _DEFAULT_WARNING_TYPE = "user:dynamic_dag_failure" | ||
|
|
||
|
|
||
| def emit_dag_warning( |
There was a problem hiding this comment.
There are no tests for task-sdk at all - this one needs to be well-covered, since it's the essence of this PR.
| session.merge(warning_to_add) | ||
|
|
||
|
|
||
| def dag_parse_failure_warnings(bundle_name: str, warnings_by_rel_path: dict[str, list]) -> list[DagWarning]: |
There was a problem hiding this comment.
Could you please rename the function so it would start with a verb?
| def _update_file_scoped_parse_failure_warnings( | ||
| files_parsed: set[tuple[str, str]], | ||
| parse_failure_warnings: Iterable[DagWarning], | ||
| session: Session, | ||
| ) -> None: |
There was a problem hiding this comment.
nit: session should be keyword-only argument
| def _update_file_scoped_parse_failure_warnings( | |
| files_parsed: set[tuple[str, str]], | |
| parse_failure_warnings: Iterable[DagWarning], | |
| session: Session, | |
| ) -> None: | |
| def _update_file_scoped_parse_failure_warnings( | |
| *, | |
| files_parsed: set[tuple[str, str]], | |
| parse_failure_warnings: Iterable[DagWarning], | |
| session: Session, | |
| ) -> None: |
|
|
||
| import { useAutoRefresh } from "src/utils"; | ||
|
|
||
| const PARSE_FAILURE_WARNING_TYPE = "user:dynamic_dag_failure"; |
There was a problem hiding this comment.
You repeat the literal user:dynamic_dag_failure few times in the code rather importing it from a single source of truth, please consider changing that where it's not necessary to repeat.
Description
Addresses #29275.
Partial failures in dynamic DAG factories/files can call
emit_dag_warning()during file parse. The DAG processor takes those warnings onDagFileParsingResult& stores them indag_warningtable (with columns -bundle_name&filename) & exposes them via GET/api/v2/dagWarnings& a Dashboard parse-failure card.Reproduced the issue using the below sample script on
main& then verified the fix on feature branch.Evidence
Sample dynamic DAG creator file :-
Was generative AI tooling used to co-author this PR?
Not generated by AI, used Cursor for approach & design finalization. Code is fully vetted by me.
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.