Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable Comms in the TriggerRunner to make Connections, Variables and XComs accessible to Triggers #48239

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Enable Comms in the TriggerRunner to make Connections, Variables and XComs accessible to Triggers #48239

wants to merge 15 commits into from

Conversation

pierrejeambrun
Copy link
Member

@pierrejeambrun pierrejeambrun commented Mar 25, 2025 •
edited
Loading

Closes: #48183

Init the comms to be able to retrieve connections, variables and xcoms from the Trigger.

@pierrejeambrun pierrejeambrun self-assigned this Mar 25, 2025
@boring-cyborg boring-cyborg bot added area:core-operators Operators, Sensors and hooks within Core Airflow area:task-sdk area:Triggerer labels Mar 25, 2025
@pierrejeambrun pierrejeambrun added area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK and removed area:core-operators Operators, Sensors and hooks within Core Airflow area:Triggerer area:task-sdk labels Mar 25, 2025
@pierrejeambrun pierrejeambrun mentioned this pull request Mar 25, 2025
Open
2 tasks
@pierrejeambrun
Copy link
Member Author

cc: @dstandish

@pierrejeambrun pierrejeambrun force-pushed the 48183-init-trigger-comms branch from 0c0521a to e3a3ea0 Compare March 25, 2025 14:08
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 25, 2025
View reviewed changes
ashb
ashb reviewed Mar 25, 2025
View reviewed changes
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the approach looks about right.

I'm a little concerned about the interleaving/mixign of cancel and conn response messages in the async side read. Let me re-read that code

@pierrejeambrun pierrejeambrun force-pushed the 48183-init-trigger-comms branch from f068d12 to 759ff89 Compare March 26, 2025 09:59
@pierrejeambrun
Copy link
Member Author

Can also retrieve Variables.

@pierrejeambrun pierrejeambrun force-pushed the 48183-init-trigger-comms branch from f11ad6f to 6f2a88d Compare March 28, 2025 11:28
@pierrejeambrun
Copy link
Member Author

Branch has been rebased and conflicts are solved but xcom part will most likely need more fixing

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Mar 28, 2025 •
edited
Loading

Locally running breeze testing core-tests --run-db-tests-only --test-type Core reproduces the CI failure (and is faster to run than the CI command). Test are interfering with each others. The test introduced in this PR, and airflow-core/tests/unit/models/test_dagrun.py::test_calls_to_verify_integrity_with_mapped_task_zero_length_at_runtime always fail.

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Mar 28, 2025 •
edited
Loading

supervisor.run() is the line causing the problem. This breaks the dag_run test somehow. Some resources must still be in use from that.

edit: supersisor.kill() is not helping. Side effect is still hapening

edit: Also we are missing from clearing the triggers and jobs in the autouse setup fixture. Adding those didn't help. (careful triggers, and other resources are not included in the clear_all I will open a PR to fix that)
PR here: #48496

@pierrejeambrun pierrejeambrun force-pushed the 48183-init-trigger-comms branch from 30c11b2 to df2ff04 Compare March 28, 2025 17:40
@ashb ashb force-pushed the 48183-init-trigger-comms branch from df2ff04 to d134e29 Compare March 28, 2025 20:50
@ashb
Copy link
Member

ashb commented Mar 28, 2025

Re-triggering tests.

@ashb ashb closed this Mar 28, 2025
@ashb ashb reopened this Mar 28, 2025
@gopidesupavan gopidesupavan reopened this Mar 28, 2025
@gopidesupavan
Copy link
Member

Triggering tests..

@pierrejeambrun pierrejeambrun force-pushed the 48183-init-trigger-comms branch from d134e29 to d2d9553 Compare March 29, 2025 02:51
@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Mar 29, 2025 •
edited
Loading

Well that's progress. test_trigger_can_access_variables_connections_and_xcoms is now working in all tests beside one timeout.

It seems that there's still one side effect to fix on test_calls_to_verify_integrity_with_mapped_task_zero_length_at_runtime

@ashb ashb force-pushed the 48183-init-trigger-comms branch from d2d9553 to d11954f Compare March 29, 2025 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashb ashb ashb approved these changes

@amoghrajesh amoghrajesh amoghrajesh left review comments

@kaxil kaxil kaxil approved these changes

@phanikumv phanikumv phanikumv approved these changes

@dstandish dstandish Awaiting requested review from dstandish dstandish is a code owner

@hussein-awala hussein-awala Awaiting requested review from hussein-awala hussein-awala is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@o-nikolas o-nikolas Awaiting requested review from o-nikolas o-nikolas is a code owner

Assignees

@pierrejeambrun pierrejeambrun

Labels
area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK
Projects
AIP-72 - Task Execution Interface and SDK
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Triggerer code does not seem to be able to use an Airflow connection & variable
6 participants
@pierrejeambrun @gopidesupavan @kaxil @ashb @amoghrajesh @phanikumv