Skip to content

Let MSGraphAsyncOperator and MSGraphSensor start from the triggerer - #53009

Open
dabla wants to merge 1 commit into
apache:mainfrom
dabla:feature/msgraph-start-from-trigger
Open

dabla wants to merge 1 commit into
apache:mainfrom
dabla:feature/msgraph-start-from-trigger

Conversation

@dabla

@dabla dabla commented Jul 8, 2025 •

Copy link
Copy Markdown
Contributor

MSGraphAsyncOperator and MSGraphSensor do all their requests in the triggerer, so the worker they start on only hands the first request over. With the new start_from_trigger=True argument the scheduler defers the task itself and that first worker run is skipped.

This PR was blocked on template rendering: a task which starts from a trigger got its trigger built from unrendered fields. That is solved since #55068 (Airflow 3.3.0), where the triggerer renders the templated fields the trigger shares with the operator. The branch is rebuilt as one commit on current main, as the msgraph code changed a lot since it was opened.

What changed since the earlier version of this PR

The earlier review asked to always start from the trigger. This version makes it opt-in instead, like FileSensor, TimeSensor, DateTimeSensorAsync and DataprocSubmitJobOperator, because starting from the trigger is not transparent for existing Dags:

  • the triggerer renders with a plain Jinja environment, without the user_defined_macros and user_defined_filters of the Dag, and it always renders strings, also with render_template_as_native_obj;
  • the task goes straight from scheduled to deferred, so the first request does not wait for a pool slot or for a worker.

When a task still starts on a worker

With the argument set, a task falls back to the worker path without failing, so it can be set through default_args:

  • on Airflow older than 3.3 (3.1 and 3.2 ignore start_from_trigger, 2.11 and 3.0 honour it but do not render templates);
  • when an argument which is passed on to the trigger is an XComArg, a callable or a file-like object: the triggerer never resolves an XComArg, and the other two cannot be serialized;
  • when the task is mapped: the class attributes stay at their defaults, so a mapped task has no start_trigger_args for the scheduler to use.

execute(), execute_complete() and the pagination are untouched; they remain the path for those cases and for every following page or poll. For the sensor the StartTriggerArgs carry the sensor timeout, since the scheduler only applies the timeout it is handed.

Tests

  • the trigger arguments which are built, and that the trigger made from them serializes identically to the one execute() defers;
  • every fallback (XComArg, also nested in a dict or list, callable, file-like object, Airflow version, mapped task);
  • with a database: DagRun.schedule_tis defers the task, the Trigger row holds the arguments and, for the sensor, the task instance gets the trigger timeout, while a task which fell back is scheduled as usual;
  • with a database: the trigger renders the templated fields once it gets its task instance, and sends the rendered request;
  • the whole flow for both classes, through tests_common.test_utils.operators.run_deferrable, which now builds and renders the trigger from start_trigger_args when an operator starts from the trigger.

Run locally on main: the msgraph operator, sensor, trigger and notifier tests, the Power BI and Analysis Services tests and the WinRM operator tests, which share the test helper (111 passed), prek pre-commit and manual stages including mypy for providers. test_generic_transfer.py of common.sql also uses the helper, but could not be run locally (no MySQL client library to build mysqlclient), so that one is left to CI.

Known limitation, not addressed here

For a task with start_from_trigger, the triggerer renders every trigger of that task, not only the one the scheduler created. The triggers execute_complete defers for the next page or the next poll hold values which were already rendered on the worker, and pass through Jinja a second time. That only matters when a rendered value itself contains Jinja markers.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Fable 5.1)

Generated-by: Claude Code (Fable 5.1) following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {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.

@dabla
dabla marked this pull request as draft July 8, 2025 09:14
@dabla

dabla commented Jul 8, 2025 •

Copy link
Copy Markdown
Contributor Author

I think there is an issue with the defer_task method of TaskInstance, it should call render_template_fields first before calling the expand_start_trigger_args method. Otherwise non rendered fields would be passed to the triggerer to instantiate, which is a not the same behaviour as with regular operators defering afterwards.

@dabla

dabla commented Jul 8, 2025

Copy link
Copy Markdown
Contributor Author

Ok overriding the expand_start_trigger_args doesn't help because the scheduler has a serialized version of the task (e.g. operator), so it won't take into account that overriden method. So now remains the question, how do we fix this issue. This is clearly a situation which hasn't been thought about.

@dabla

dabla commented Jul 9, 2025

Copy link
Copy Markdown
Contributor Author

This PR must be merged before this one can.

@dabla dabla mentioned this pull request Aug 13, 2025
2 tasks done
@dabla

dabla commented Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Created new PR which attempts to fix template rendering with start from trigger, as long as template rendering in triggerer isn't fixed, this PR can't be merged.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 12, 2026
@github-actions github-actions Bot closed this Apr 5, 2026
@dabla dabla reopened this Oct 5, 2026
@dabla
dabla force-pushed the feature/msgraph-start-from-trigger branch from 0ea5e5a to 6ce73ff Compare October 5, 2026 06:51
@dabla dabla removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Oct 5, 2026
@dabla dabla changed the title Allow MSGraphAsyncOperator and MSGraphSensor to directly start from trigger instead of worker Let MSGraphAsyncOperator and MSGraphSensor start from the triggerer Oct 5, 2026
@dabla dabla self-assigned this Oct 5, 2026
@dabla
dabla requested review from eladkal and shahar1 October 5, 2026 06:52
Both do all their requests in the triggerer, so the worker they start on
only hands the first request over and that round trip is wasted. Since
Airflow 3.3 the triggerer renders the templated fields of a task which
starts from a trigger, which was what kept these two from using it.

It is opt-in because the triggerer renders with a plain Jinja environment,
without the user defined macros and filters or the native rendering of the
Dag, and the task no longer waits for a pool slot. A task falls back to a
worker when it cannot start from the trigger: on older Airflow versions,
when an argument is an XComArg, a callable or a file-like object, which
the triggerer cannot resolve or receive, and when it is mapped.
@dabla
dabla force-pushed the feature/msgraph-start-from-trigger branch from 6ce73ff to b8afc1f Compare October 5, 2026 18:00
@dabla
dabla marked this pull request as ready for review October 5, 2026 18:12
@dabla
dabla requested a review from ashb October 6, 2026 08:15

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants