One question: are we deprecating the existing parameters straight away or leaving it without a warning for one release? (I think someone talked about that at one point)
On 6 August 2022 19:46:02 BST, Daniel Standish <daniel.stand...@astronomer.io.INVALID> wrote: >Well, thanks for forcing us to make the case :) > >I'll initiate a lazy consensus "vote" > > >On Fri, Aug 5, 2022 at 1:01 PM Vikram Koka <vik...@astronomer.io.invalid> >wrote: > >> Fair points all. >> I withdraw my objection and agree with the consolidation proposal. >> >> Vikram >> >> >> On Fri, Aug 5, 2022 at 10:06 AM Jarek Potiuk <ja...@potiuk.com> wrote: >> >>> Let me also add to why I think we should consolidate to one param:: >>> >>> * "schedule_interval" >>> * "schedule_on" >>> * "timetable" >>> >>> Imagine you are a new user coming to Airflow. >>> >>> Do you immediately recognise which one is which ? Do you see what the >>> difference is? >>> >>> * "Schedule_on" = "Every Sunday" is equally good as "schedule_on" >>> "dataset". >>> * "timetable" - what even is a timetable? Is it a "cron TAB" ? Or what? >>> >>> For me even if we wanted to have different names, the current set of >>> names for those params is just terribly ambiguous. >>> >>> Making it "schedule" and acting depending on what you pass to it is so >>> much better and simpler. >>> >>> J. >>> >>> >>> On Fri, Aug 5, 2022 at 6:57 PM Daniel Standish >>> <daniel.stand...@astronomer.io.invalid> wrote: >>> >>>> @Vikram, yeah, I understand that concern. Making a breaking change is >>>> something we shouldn't do carelessly, without consideration, without good >>>> reason. But I think we have a good reason here. >>>> >>>> Deprecations are somewhat regular in airflow. In 2.0 we moved a lot of >>>> imports. We also changed the CLI semantics. Probably there were other >>>> breaking changes that I'm forgetting *(granted sometimes we didn't >>>> remove the backward-compatibility immediately in 2.0, and that remains an >>>> option here)*. Probably just about every dag in existence had to be >>>> changed then too. And that's what a major release is for. >>>> >>>> In 3.0, it looks like we're deprecating execution_date... That's even >>>> bigger. So, were we to do this, (1) moving to `schedule` wouldn't be the >>>> only notable breaking change and (2) by comparison it's a pretty simple >>>> change. And if, as part of the upgrade to a major release, you have to do >>>> renames and such anyway, the marginal cost of one more isn't massive. >>>> >>>> I guess my take is that we can't be chained to our previous decisions >>>> forever. And we have to have the flexibility to adapt our interfaces so >>>> that they still make sense and are consistent and don't cause excessive >>>> eyebrow wrinkling. We have adopted semantic versioning to provide a >>>> mechanism for signaling when breaking changes are coming. And we can add >>>> tooling to help users along the way. >>>> >>>> Additionally, we can't *only *look at the burden of such a change for >>>> users. We must also weigh this against the burden that we maintain by >>>> *not* changing, i.e. by having too many params, or ill-named params. >>>> >>>> >>>> On Thu, Aug 4, 2022 at 2:57 PM Jed Cunningham <jedcunning...@apache.org> >>>> wrote: >>>> >>>>> I'm not in favor of reusing `schedule_interval` for datasets (or >>>>> folding `timetable` into it either), primarily because it doesn't feel >>>>> like >>>>> a good name to describe what it does. If we don't have the appetite to >>>>> consolidate, my vote is we stick with 3 separate params. So preferably #1, >>>>> if not then #2 for me. >>>>> >>>>> I'm less concerned about the "everyone has to update every DAG" issue, >>>>> as it only becomes a problem for Airflow 3, folks can freely ignore it >>>>> until then, and when they do update it it's a simple find/replace. >>>>> >>>>> >>>>> On Thu, Aug 4, 2022 at 2:17 PM Andrew Gibbs < >>>>> a.r.gibbs.massma...@gmail.com> wrote: >>>>> >>>>>> I'll just +1 that IMO a UI reminder is a great option. >>>>>> >>>>>> 2.3.0 added an alert when robots.txt was being scanned, which was >>>>>> great, because I ignored it for a while, but eventually caved and looked >>>>>> into it. This I would see as more of the same. I currently get loads of >>>>>> warnings about things in my config file that have moved; but typically >>>>>> only >>>>>> if I have to do a `airflow db upgrade` or `airflow dags backfill`. I see >>>>>> the alert, think "I must fix those", and then the Homer effect kicks in, >>>>>> and something else takes up the space in my brain. Having them as a >>>>>> banner >>>>>> in the GUI would 100% get me to sort it sooner or later. >>>>>> >>>>>> On Thu, Aug 4, 2022 at 9:32 PM Vikram Koka >>>>>> <vik...@astronomer.io.invalid> wrote: >>>>>> >>>>>>> Hi Daniel, >>>>>>> >>>>>>> I completely agree with wanting to consolidate, but I am very >>>>>>> concerned about people needing to change their existing code. >>>>>>> As Ping and Felix also said, I worry that people ignore deprecation >>>>>>> warnings, especially since the original code authors don't really look >>>>>>> at >>>>>>> logs or other pipeline artifacts unless something is broken. >>>>>>> >>>>>>> With respect to the options you outlined, I am personally leaning >>>>>>> towards (4) i.e. "don't deprecate timetable, but remove schedule_on >>>>>>> (AIP-48 >>>>>>> has added it in main) and take List[Dataset] in `schedule_interval`". I >>>>>>> could be convinced with (3), since I think that "timetable" is >>>>>>> relatively >>>>>>> recent and have not seen as much usage for this invocation yet. I am >>>>>>> definitely curious about other perspectives on this as well. >>>>>>> >>>>>>> I have a hard time with option (1) of deprecating "schedule >>>>>>> interval", because of the historical nature of it and the code changes >>>>>>> that >>>>>>> people may have to make to deal with it. >>>>>>> >>>>>>> Thanks for raising this topic and the discussion, >>>>>>> >>>>>>> Vikram >>>>>>> >>>>>>> >>>>>>> On Thu, Aug 4, 2022 at 10:24 AM Daniel Standish >>>>>>> <daniel.stand...@astronomer.io.invalid> wrote: >>>>>>> >>>>>>>> I agree with those (including myself) who have suggested that it >>>>>>>> would be great to have a deprecation fix tool. >>>>>>>> >>>>>>>> I also think that UI notifications would be valuable. Another >>>>>>>> option would be to provide a less intrusive indicator somewhere (like >>>>>>>> when >>>>>>>> chrome lets you know you need to update), and then you click on it and >>>>>>>> it >>>>>>>> takes you to a page that lists out all the deprecation warnings. >>>>>>>> >>>>>>>> >>>>>>>> On Thu, Aug 4, 2022 at 1:03 AM Jarek Potiuk <ja...@potiuk.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> I know we are distracting a bit from the main subject, but I think >>>>>>>>> it's worth it. >>>>>>>>> >>>>>>>>> I think this is a fact of life that people don't remove >>>>>>>>> deprecations. But we have a choice of "not doing anything about it" >>>>>>>>> and >>>>>>>>> complaining. Or doing something that might improve it. >>>>>>>>> >>>>>>>>> I usually err on the side of "doing something" when I see a >>>>>>>>> problem. I am a doer :). >>>>>>>>> >>>>>>>>> And I have a possibly controversial proposal. I got the inspiration >>>>>>>>> from node.js old versions. Whenever an old version of Node is being >>>>>>>>> used, >>>>>>>>> you have a 20 seconds delay artificially added and information that in >>>>>>>>> order to remove those 20 seconds you need to upgrade. >>>>>>>>> >>>>>>>>> Why don't we do that for some really old/serious deprecations ? >>>>>>>>> Maybe not 20 seconds. Maybe adding an unremovable RED warning in the >>>>>>>>> UI if >>>>>>>>> you have any deprecation unhandled. Something that is annoying enough >>>>>>>>> that >>>>>>>>> will cause people to make an action but not intrusive enough to break >>>>>>>>> things. >>>>>>>>> >>>>>>>>> I think this is a very basic psychological trait: >>>>>>>>> >>>>>>>>> If it does not cost the user anything to leave the deprecations, >>>>>>>>> there will be a big number of people who will do that. >>>>>>>>> We need to make sure that deprecations are costing our users "a >>>>>>>>> little" and that they are aware that they can remove the cost (and >>>>>>>>> know how >>>>>>>>> - this is super important that we only show such a message with clear >>>>>>>>> instructions on what to do). >>>>>>>>> >>>>>>>>> Such a message might be annoying in many ways: >>>>>>>>> >>>>>>>>> * it can obstruct significant part of the screen >>>>>>>>> * It can be flashy and had very annoying colours (think bold pink >>>>>>>>> fonts on flash-green background for example) >>>>>>>>> * It can not allow to do anything before the user closes it with a >>>>>>>>> REALLY small 'x' that it is difficult to point at (think cookie >>>>>>>>> banners). >>>>>>>>> And when they click they can be greeted with "You know - you can >>>>>>>>> actually >>>>>>>>> remove this warning by doing THIS". >>>>>>>>> * and we have probably more options - our UX specialists might have >>>>>>>>> some good ideas here >>>>>>>>> >>>>>>>>> For anyone who complains about "people do not act on deprecation." >>>>>>>>> - yeah this is true. >>>>>>>>> But one universal truth there is that if we won't act, this will >>>>>>>>> change nothing in the behaviour of our users - this is 100% certain. >>>>>>>>> >>>>>>>>> If we do something - it might improve it. We will not get a 100% >>>>>>>>> success for sure, but we can at least do something to improve it. >>>>>>>>> >>>>>>>>> WDYT? >>>>>>>>> >>>>>>>>> J. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Thu, Aug 4, 2022 at 6:52 AM Felix Uellendall >>>>>>>>> <felue...@pm.me.invalid> wrote: >>>>>>>>> >>>>>>>>>> Hi Ping, >>>>>>>>>> >>>>>>>>>> I had this idea about auto-fixing deprecation warnings too when I >>>>>>>>>> started the airflint project. >>>>>>>>>> I totally agree with what you said that often people ignore these >>>>>>>>>> warnings and therefore a cli tool would come in very handy. It could >>>>>>>>>> look >>>>>>>>>> similar to the airflint project and also be based on the python >>>>>>>>>> refactor >>>>>>>>>> package, but I would keep it out of this discussion for now. >>>>>>>>>> >>>>>>>>>> Regarding the schedule arg, I don’t have a strong opinion there >>>>>>>>>> but also tend to having one schedule arg, because of simplicity >>>>>>>>>> for the dag author. >>>>>>>>>> >>>>>>>>>> Best, >>>>>>>>>> Felix >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Sent from Proton Mail for iOS >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Thu, Aug 4, 2022 at 00:23, Ping Zhang <pin...@umich.edu> wrote: >>>>>>>>>> >>>>>>>>>> Hi Daniel, >>>>>>>>>> >>>>>>>>>> +1 for ‘schedule' and thanks for bringing this up. >>>>>>>>>> >>>>>>>>>> I agree with Vikram, we should be very careful about deprecating >>>>>>>>>> existing params even if we have warnings around it. Not sure if this >>>>>>>>>> is a >>>>>>>>>> general case, but I notice that people ignore warnings all the time >>>>>>>>>> until >>>>>>>>>> it starts to break. Thus I am thinking if airflow can have a cli >>>>>>>>>> tool to >>>>>>>>>> help users to batch update these deprecating params just like >>>>>>>>>> python's >>>>>>>>>> futurize tool. This can greatly ease the burden of upgrading the >>>>>>>>>> airflow >>>>>>>>>> version. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> >>>>>>>>>> Ping >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Aug 3, 2022 at 3:13 PM Daniel Standish >>>>>>>>>> <daniel.stand...@astronomer.io.invalid> wrote: >>>>>>>>>> >>>>>>>>>>> In case you forgot about us @Vikram, gentle nudge >>>>>>>>>>> >>>>>>>>>>> On Mon, Aug 1, 2022 at 8:40 AM Daniel Standish < >>>>>>>>>>> daniel.stand...@astronomer.io> wrote: >>>>>>>>>>> >>>>>>>>>>>> Well, this is a discussion thread, so let's discuss! >>>>>>>>>>>> >>>>>>>>>>>> Vikram, what kind of implications are you thinking of? Maybe you >>>>>>>>>>>> could provide a little more detail about your concerns? >>>>>>>>>>>> >>>>>>>>>>>> There are certainly alternatives, and of course each of them has >>>>>>>>>>>> tradeoffs. >>>>>>>>>>>> >>>>>>>>>>>> Here are the options I see: >>>>>>>>>>>> >>>>>>>>>>>> 1. consolidate to schedule (deprecate schedule_interval and >>>>>>>>>>>> timetable; the proposal under discussion in this thread) >>>>>>>>>>>> 2. do nothing (keep 3 different scheduling params) >>>>>>>>>>>> 3. consolidate to schedule_interval (deprecate timetable) >>>>>>>>>>>> 4. don't deprecate timetable, but remove schedule_on (AIP-48 has >>>>>>>>>>>> added it in main) and take List[Dataset] in `schedule_interval` >>>>>>>>>>>> >>>>>>>>>>>> Which would you propose we go with? >>>>>>>>>>>> >>>>>>>>>>>> If we have a veto against 1 (and again, we're in hypothetical >>>>>>>>>>>> territory since this is still a discussion thread), my vote would >>>>>>>>>>>> definitely be to go with 3. >>>>>>>>>>>> >>>>>>>>>>>> The param `schedule_interval` as catch all is not *sooo *bad, >>>>>>>>>>>> but `schedule` definitely seems better. From a new user's >>>>>>>>>>>> perspective, >>>>>>>>>>>> having misleading names is confusing, and can be a negative when >>>>>>>>>>>> looking at >>>>>>>>>>>> adoption. And for a tenured user, it can nag at you. And dealing >>>>>>>>>>>> with >>>>>>>>>>>> deprecations as an Airflow user, well it's sort of part of the >>>>>>>>>>>> job, though >>>>>>>>>>>> it can be bothersome, certainly. >>>>>>>>>>>> >>>>>>>>>>>> Just to look at / compare feasibility. >>>>>>>>>>>> >>>>>>>>>>>> Deprecating schedule_interval, it would be more prominent than >>>>>>>>>>>> some other deprecations because it's a DAG param. That said, I >>>>>>>>>>>> think it's >>>>>>>>>>>> pretty straightforward to handle. For one, it's at the cardinality >>>>>>>>>>>> of DAG >>>>>>>>>>>> and not *task*, so there is less to change with than some other >>>>>>>>>>>> deprecations. And being kwargs-only, it's an easy find and replace >>>>>>>>>>>> that >>>>>>>>>>>> could be automated with an `upgrade-check` script. It's very easy >>>>>>>>>>>> to >>>>>>>>>>>> detect, either to warn about, or ultimately, after upgrade, to >>>>>>>>>>>> fail the DAG >>>>>>>>>>>> parse (which forces user to fix). Perhaps we could even experiment >>>>>>>>>>>> with a >>>>>>>>>>>> deprecation fix tool, so that users could get help fixing >>>>>>>>>>>> deprecations, not >>>>>>>>>>>> necessarily in the context of an upgrade, since we're not looking >>>>>>>>>>>> at 3.0 >>>>>>>>>>>> any time soon. >>>>>>>>>>>> >>>>>>>>>>>> We could compare with execution_date, if execution_date is to be >>>>>>>>>>>> removed in 3.0. I think the deprecation of execution_date will be >>>>>>>>>>>> a more >>>>>>>>>>>> difficult thing for users to handle. For one, potentially higher >>>>>>>>>>>> cardinality, since it is used in tasks. But also, the use of >>>>>>>>>>>> execution_date >>>>>>>>>>>> can be harder to detect, since it can be in templates and >>>>>>>>>>>> elsewhere in >>>>>>>>>>>> custom operators. *(With schedule_interval, since it's a >>>>>>>>>>>> top-level dag kwarg, it will be easier.)* And execution_date >>>>>>>>>>>> was replaced with `logical_date` for similar reasons -- avoiding >>>>>>>>>>>> user >>>>>>>>>>>> confusion. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Fri, Jul 29, 2022 at 2:42 PM Vikram Koka >>>>>>>>>>>> <vik...@astronomer.io.invalid> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> -1 from me. >>>>>>>>>>>>> >>>>>>>>>>>>> Though I agree in principle with the idea of consolidation, I >>>>>>>>>>>>> don't think we should be doing this yet until we understand the >>>>>>>>>>>>> implications completely. >>>>>>>>>>>>> I am really not in favor of deprecation of the existing params, >>>>>>>>>>>>> unless there is really no alternative. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Jul 29, 2022 at 2:37 PM Daniel Standish >>>>>>>>>>>>> <daniel.stand...@astronomer.io.invalid> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> So far, seems all in favor. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I will just highlight, in case it's not clear.... when we >>>>>>>>>>>>>> release this (presumably 2.4), basically every single dag in >>>>>>>>>>>>>> existence will >>>>>>>>>>>>>> start emitting deprecation warnings, and prior to 3.0, basically >>>>>>>>>>>>>> every dag >>>>>>>>>>>>>> in existence will need to be updated. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Thankfully, for most people, this should be an easy update >>>>>>>>>>>>>> since DAG is kwargs only, IIRC. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Fri, Jul 29, 2022, 2:27 PM Brent Bovenzi >>>>>>>>>>>>>> <br...@astronomer.io.invalid> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> +1 to consolidating and "schedule". >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Fri, Jul 29, 2022, 10:12 PM Drew Hubl >>>>>>>>>>>>>>> <drew.h...@astronomer.io.invalid> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> +1 for ‘schedule', and another +1 to the importance of >>>>>>>>>>>>>>>> consolidating to one being more important than the name of >>>>>>>>>>>>>>>> that one >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Jul 29, 2022, at 1:58 PM, Josh Fell < >>>>>>>>>>>>>>>> josh.d.f...@astronomer.io.INVALID> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Consolidating to a single scheduling parameter is a big +1 >>>>>>>>>>>>>>>> from me. `schedule` seems like a nice catch-all. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Fri, Jul 29, 2022 at 9:10 AM Jed Cunningham < >>>>>>>>>>>>>>>> jedcunning...@apache.org> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> +1 on moving to a single `schedule` param. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>
