Skip to content

Conversation

@amoghrajesh
Copy link
Contributor

@amoghrajesh amoghrajesh commented Sep 17, 2025

closes: #54712

Why?

Task SDK uses the ability to send emails but it imports it from airflow.utils. As part of client server separation, getting rid of these imports in task sdk. This PR moves the _send_email function from utils to the task runner module to consolidate email-related functionality where it's actually used.

Changes

  • Moved _send_email function from utils to task runner
  • Added test to verify email functionality works correctly in the new location -- there was no task test that did this

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

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.

I wonder if we could replace this with the SMTP provider's SmtpNotifier in some way?

It seems odd to have two different ways of sending email.

We could fail at parse time if you set email_on_failure = True and the smtp provider can't be imported?

@amoghrajesh
Copy link
Contributor Author

@ashb we could, but do we want to create a dependency between task sdk and smtp provider? I did this to avoid that

@ashb
Copy link
Member

ashb commented Sep 17, 2025

I think there is a qausi-dep there already -- SMTP provider was core, and you can define the credentials in an SMTP connection inside Airflow, can't you?

This doesn't need to be a hard (i.e. pyproject-level) dependency

@ashb
Copy link
Member

ashb commented Sep 17, 2025

I've thought about it before, and changing email_on_failure automatically to an SMTPNotifier seems like the way to go here. And if you set email_on_failure and the provider isn't importable, we can raise a nice error message to show up as a dag parse errror

@kaxil
Copy link
Member

kaxil commented Sep 17, 2025

@eladkal eladkal modified the milestones: Airflow 3.2+, Airflow 3.2.0 Sep 23, 2025
@eladkal
Copy link
Contributor

eladkal commented Sep 23, 2025

I am really not sure where are we going with the email/smtp integration.
My wish to get rid of it in Airflow 3 and treat it like any other notifier out there was not successful.
I am hoping we can get to a state where all email core related stuff are 100% deprecated and advise users migrate to the SMTP notifier. I don't think we need to continue build things in task sdk for emails.

@ashb
Copy link
Member

ashb commented Sep 23, 2025

Lets not port this over then.

Instead lets "upgrade in place" the use of email_on_failure attr etc and automatically convert that to an SmtpNotifier instance. And if the smtp provider isn't installed raise a nice error of "You need to install apache-airflow-providers-smtp to use this feature".

I'll leave @amoghrajesh do decide if this PR gets closed or re-purposed.

@ashb ashb marked this pull request as draft September 23, 2025 13:12
@amoghrajesh
Copy link
Contributor Author

Thanks for all your comment, let's repurpose the existing SMTP notifier to do the email sending portion for us

@amoghrajesh
Copy link
Contributor Author

Closing in favour of #57354

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

Development

Successfully merging this pull request may close these issues.

Remove airflow.utils.email references in task-sdk

4 participants