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 >>
