Yeah. Combining that in a single commit is easy (I actually had it before when I started the discussion https://github.com/apache/airflow/pull/24816 /.
I only changed it based on TP's comments that it is 'too big to review" :). But I propose we do it in the way that review is done individually for each PR and after that I combine it in a single (or rather two - separate one for providers) PR. That would likely be the "best of both worlds". However, before I proceed with it, I want to do it "properly" and ask others. I do not want to make it without the community's consent. Do you think we should do it? Do you find it as useful as I do ? One more comment here - the main reason why I wanted to do it is that I really do not like is that currently we already have inconsistent approach: we currently have 9 packages in "core" airflow that are already using __future__.annotations - they've been apparently added without any consensus and thinking about consistency: - and I find it terribly annoying that sometimes i have to use (param: list(str) and sometimes params: List[str]) - depending on which file I am in. We currently have __future__.annotaitons added in: * entry_points.py * airflow/__init__.py * airflow/models/datasets.py * expandinput.py * outputs.py * templates.py * _cron.py * timetables/trigger.py If there will be no strong opposition, I will ask for a lazy consensus and I will rebase it and will ask for quick reviews on those 47 PRs and combine them into two eventually. I will also cherry-pick them carefully to v2-4 branch, making sure that I do not drag-along unwanted changes from main. TP - I hope it all (the discussion and proposal) alleviates your concerns? J. On Mon, Sep 12, 2022 at 11:10 AM Ash Berlin-Taylor <[email protected]> wrote: > Changes are fine (Other than adding the annotation to empty __init__ > files), but having this as 47 PRs I think is unhelpful when it comes to > spelunking changes in Git History (i.e. using `git blame`, which I at least > seem to do once every couple of weeks) so I would prefer it there was 1 for > core airflow, and one for providers, say. > > I don't feel that strongly about this, it's just how I would have done it > personally -- and having a single rev lets me more easily ignore that rev. > > -ash > > On Sun, Sep 11 2022 at 14:29:56 +01:00:00, Ash Berlin-Taylor < > [email protected]> wrote: > > Right okay, gotcha. I'll take a look tomorrow if no one does before hand. > > On 11 September 2022 13:33:27 BST, Jarek Potiuk <[email protected]> wrote: >> >> Yeah. It was a bit late. But this was the main reasoning - if we merge >> them quickly AND cherry-pick those to 2.4 branch we have a very good chance >> to avoid any accidental merge of something that was merged since the 2.4 >> branch cut-off - there were literally a handful of those PRs merged and it >> will be immediately visible (this is also one reason why I split it into >> many small PRs to detect and fix any kind of problems and remove just >> merged code while cherry-picking). >> >> This was the reasoning behind doing it fast when I recalled that we hold >> off from it in the middle of 2.3 development. >> >> J,. >> >> >> On Sun, Sep 11, 2022 at 1:19 PM Ash Berlin-Taylor <[email protected]> wrote: >> >>> One comment on the timing: but doing it now you make cherry picks for >>> 2.4.x harder, of which were about to do a lot of. The best time to do such >>> a change is just _before_ cutting the next main branch. >>> >>> On 10 September 2022 21:38:12 BST, Jarek Potiuk <[email protected]> >>> wrote: >>>> >>>> Hey Everyone, >>>> >>>> TL;DR; I planned to ask for help in reviewing the 48 (pretty much >>>> fully automated PRs I prepared, but a comment from TP made me think that >>>> maybe I went too much ahead and maybe there are other ideas on implementing >>>> the change (or even maybe not implementing it at all). >>>> >>>> More context: >>>> >>>> I have just attempted what I THOUGHT was generally accepted and good >>>> idea, but comment from TP made me think maybe I went ahead of everyone >>>> else, so I am reaching here to see if this was a good idea (And I am ready >>>> to back-out and do it differently - and follow another way if someone can >>>> propose it). >>>> >>>> I just (literally while watching Poland <-> Brazil Volleybal World >>>> Championship semi-finals) prepared 48 separate PRs to migrate to >>>> __future__.annotations which (I thought) was considered a good idea and >>>> something we wanted to defer to after v2-4-test branch cut-off because of >>>> the concerns that it might make cherry-picking more complex. It was 9X% >>>> fully automated conversion - by search/replace and adding `from __future__ >>>> import annotations` and letting upgrade do the job (as discussed before). >>>> I also split the one huge PR into 48 (!) much smaller PRs - basically >>>> one per each "airflow" core package, few big providers separately, tests >>>> separately (as they are far less risky than "core code" change). >>>> >>>> We already added __future__ annotation in a number of other places >>>> before - without a "broad" agreement and consensus/voting (different people >>>> did in various places - including myself and others) so I thought this is >>>> not something we need a broader voting/consensus on. >>>> >>>> Again - that was 95% automated change where I contributed mostly the >>>> thinking "how to split '' theI PR and all the rest was literally "git >>>> commit add <package>" (and let pyupgrade and our other pre-commits like >>>> isort and others) do the job. >>>> >>>> It never occured to me that this might be a problem for anyone. My >>>> understanding of the problem was: >>>> * we want to do it >>>> * we were afraid to do it before because it would make cherry-picking >>>> to 2-3 branch more problematic >>>> * we wanted to do it right "when" we cut-off the 2.4 branch (for the >>>> same reason as above - we wanted to avoid cherry-picking problems for 2.4.* >>>> changes) >>>> >>>> So I wanted to make this change as fast as humanly possible - and raise >>>> this multiple PRs so we can review/approve (and I'd cherry-pick them) as >>>> soon as possible to minimise the cherry-picking problem. >>>> >>>> But maybe I was too fast and it's not as straightforward as I thought >>>> it was? (actually it never occured to me that there might be a slightest >>>> problem with it after the earlier discussion). I literally thought about it >>>> as a mechanical change that we want to introduce and that doing that in >>>> small chunks (as discussed in the original PR) was the best approach. The >>>> comment from TP >>>> https://apache-airflow.slack.com/archives/CCPRP7943/p1662837864413849 >>>> - made me think that I have misjudged it. >>>> >>>> I wanted to write that email asking all of the committers here to help >>>> to review and merge all those PRs as fast as possible (again - those are >>>> purely mechanical changes, there are literally a few - literally less than >>>> 5) manual changes I had to make to fix some mypy problems. >>>> All the PRs are here: >>>> https://github.com/apache/airflow/pulls?q=is%3Aopen+is%3Apr+label%3Afuture-annotations >>>> >>>> But then I ask for something else then: >>>> >>>> * do you think this is fine we add __future__ annotations in general >>>> * do you think the way I proposed it is somewhat too In-human (i >>>> honestly feel it is an in-human change - because it is fully automated by >>>> our tools :D). But is it wrong approach to have 48 PRs automatically >>>> prepared by our tools to get a "consistent" approach >>>> * did I do it too fast/too early ? >>>> * any ideas on how we can do it differently to avoid the >>>> "cherry-picking" problems (mentioned by others in the original message)? >>>> >>>> I am really concerned to ask if this is something I have done wrongly >>>> or misunderstood the consensus, and maybe chose the wrong way ? Maybe I >>>> should approach it differently and ask for consensus on such a flurry of >>>> PRS? Or maybe the timing is wrong. >>>> >>>> I am happy to follow in any way there - I can either keep the PRs and >>>> rebase them until the time is good (it takes about two minutes to rebase >>>> every one of them and apply the automation again) if the way I proposed it >>>> was because I misunderstood the "sentiment" about the change - or follow >>>> any other way that will help us to implement the change - or even abandon >>>> it completely if we will agree it's not a good idea. I am also happy to >>>> apply any "general comments" there - now that the PR's are there, applying >>>> any automation even on all of them to improve it, is rather simple. >>>> >>>> Looking forward to any comments. I am really curious how others look at >>>> it - regardless of the outcome - always eager to learn new things :) >>>> >>>> J. >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> On Thu, Jul 7, 2022 at 8:52 AM Tobiasz Kędzierski < >>>> [email protected]> wrote: >>>> >>>>> Great idea. +1 from me. Thanks for raising this up Jarek and thanks >>>>> to Pierre for great isort tip :) >>>>> >>>>> BR >>>>> Tobiasz >>>>> >>>>
