Change send_mail() and EmailMessage() to kwargs-only?

I’ve updated the ticket to require deprecation, as I had originally proposed.

I’ve also opened a proper PR with an updated @deprecate_posargs decorator and tests: Refs #36163 -- Implemented deprecate_posargs decorator. by medmunds · Pull Request #19145 · django/django · GitHub.

I can try to address some of the concerns that were raised about deprecation:

[my paraphrasing] When we’ve done this before, it took a while to shake out all the corner cases and bugs.

My hope in introducing a @deprecate_posargs decorator is to have a single, well-tested piece of code that considers all the corner cases. (And that provides a single place to fix future bugs we may discover.)

Adam’s earlier comment already uncovered one issue I’d missed. I’ll also look at the housekeeping package’s similar decorator to see if there are some others. (That’s why the PR is still marked draft.)

When we’ve done this before, the actual function signatures were changed to use generic, variable params during the deprecation period. That can be confusing.

With the decorator approach, we update the functions to their new, non-generic signatures immediately. (And then the decorator remaps args during the deprecation period.)

Both VS Code and PyCharm seem to handle the decorated function as having its “real” signature. (Added benefit: they flag deprecated usage as invalid.)

VS Code

PyCharm

I’ve also added a test case to verify the decorator preserves the original function’s inspect.signature().

True, plus the decorator itself adds the overhead of an additional stack frame.

The overhead can be significant for functions that do almost nothing. As they do more work, the overhead rapidly becomes immaterial. I’m guessing that most of the functions we’d want to switch to keyword-only params have a large number of params and tend to do a lot of work. But we’d need to be careful about using the decorator indiscriminately on performance-critical paths.

For the email functions, the overhead shouldn't be a concern. In some local testing on send_mail(), I found the impact of the decorator was (literally) lost in the noise.

On my development machine, with send_mail() modified to require keyword only args as proposed, I used timeit on:

send_mail("subject", "message", "from@example.com", ["to@example.com"])

Without the decorator, this averaged ~120 µsec. That breaks down as:

  • 0.2 µsec for the send_mail() call itself (tested by replacing its implementation with pass)
  • 4 µsec to instantiate an EmailBackend and call its send_messages() (tested using the dummy backend)
  • ~25 µsec to serialize the message (tested using the locmem backend modified to skip mail.outbox)
  • ~90 µsec or more to open a connection to a server and transmit the serialized message (simulated by using the console backend modified to write to /dev/null; an actual SMTP server connection would take even longer)

(The first two components were quite consistent over several runs; the latter two had a high degree of variability.)

With the decorator, the added overhead varied with the calling args:

  • 0.25 µsec decorator overhead for send_mail("subject", "message", "from@example.com", ["to@example.com"])
  • 0.42 µsec adding a couple of keyword args: send_mail("subject", "message", "from@example.com", ["to@example.com"], fail_silently=True, html_message="html")
  • 0.57 µsec changing all args to keyword: send_mail(subject="subject", message="message", ... html_message="html")

That’s less than half a percent overhead.

What’s more, to measure the decorator’s precise timing, I had to use send_mail() with its implementation replaced by pass. With the real backends, the timing delta between the decorated and non-decorated send_mail() was too variable to be helpful. In some test runs, the decorated version was actually faster. (The I/O and memory costs of shuffling serialized messages around is so variable, the added overhead from the decorator was just lost.)

The absolute numbers here are not meaningful, but the relative ones are. I also ran some (simpler) performance tests through the PR builds, and the relative results across all build environments were consistent with each other and with what I saw locally.

1 Like