Conversation
No behaviour change beyond the deprecation warning. ``DeadlineReference.register_custom_reference`` name is now misleading. A caller reading the name would believe they have registered the reference when they now need to refisger a plugin as well, and the docstring carried a ``.. warning::`` saying so. Rename it to ``_add_custom_reference_to_namespace``, which says what it does and marks it private: the documented entry point is the ``@deadline_reference`` decorator, and only that decorator and tests call it. The old name is kept as a deprecation shim so nothing breaks.
Previously, if there were two custom references with the same name the one which happened to be parsed first gor silently overwritten. We now check for collisions, and reject the shorthand reservation for both on a collision along with a warning log. Both will still be available for import, neither will get the DeadlineReference.MyReference shorthand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge Note: This is gated and must merge AFTER #70370, #68917, #70714, and #71850 have all merged
This is a bug that came out of the Airflow Summit workshop, with a couple of small related changes along for the ride.
Bug: when a user has two custom DeadlineReferences with the same class name, the one which happened to be parsed first got silently overwritten by the second. Worse than losing the shorthand, the surviving instance serializes under its own qualname, so the scheduler evaluates the wrong one. Nothing was logged above INFO, and the message that was emitted was not helpful since it only included name, which is identical between the two conflicting references.
With this change neither one gets a claim on the
DeadlineReference.MyReferenceshorthand, a warning is logged which names both classes by qualname, and both remain fully usable by importing and instantiating the class directly. I decided to withhold the shorthand notation from both rather than awarding it to the first since, before this change, a Dag written using the short name would mysteriously start using whichever reference happened to parse first after each restart, based on the arbitrary import order.Two smaller behaviour changes are included as off-shoots. A custom reference whose name collides with something already on
DeadlineReference, such asDAGRUN_QUEUED_ATorTYPES, now raises rather than replacing it, since unbinding a built-in would break every Dag using it. And registering a reference now replaces itsDeadlineReference.TYPESentry instead of appending another. This means that re-registering an edited class is now self-cleaning and self-updating; it no longer leaves duplicates behind. Both comparisons are based off the module-qualified name because a re-parse produces a new class object for the same reference and__name__alone cannot tell two modules apart.While I was in there, I renamed
register_custom_reference, which predates Jarek's PR (#66737) which enforces registration as a plugin. The naming was awkward and slightly misleading to begin with, and became outright confusing once users were unsure what exactly they were registering (registering a plugin vs registering the reference shorthand). The method is undocumented and public only by accident: it is absent fromairflow.sdk.__all__. I still added a deprecation shim and gave the new name a leading underscore just to be safe but they are likely unnecessary and the new name with the leading underscore is enforcing the original intent.The docs also only ever showed the shorthand version, so I have added the direct import-and-instantiate form to
howto/deadline-alerts.rstalong with a note about the contested-name case, since that is now the path a user gets pointed at.I split the mechanical rename, the functional name-claiming change, and the doc change into three different commits to hopefully make the review easier.
Was generative AI tooling used to co-author this PR?
Claude Opus 5 generated the tests, which I then tweaked.
{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.