Okay, yeah having reviewed the big PR (all providers apart from the big three) with over 300 files changes, yeah splitting it out for review was the right call!
I have reviewed most of them (and was able to _actually_ review them) and am just finishing off the remaining ones now. -ash On Sep 12 2022, at 11:05 am, Jarek Potiuk <[email protected]> wrote: > 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] > (mailto:[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] (mailto:[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] > > > (mailto:[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] > > > > (mailto:[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] > > > > > (mailto:[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] (mailto:[email protected])> > > > > > > wrote: > > > > > > > Great idea. +1 from me. Thanks for raising this up Jarek and > > > > > > > thanks to Pierre for great isort tip :) > > > > > > > > > > > > > > BR > > > > > > > Tobiasz > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
