And yes Ash -> ruff rule it is to ban "providers" imports https://github.com/apache/airflow/pull/46801 (and it found two places where they were still used)
On Sun, Feb 16, 2025 at 12:35 PM Jarek Potiuk <ja...@potiuk.com> wrote: > And merged! that was fast. Thanks Bugra, Ash, Jens ! > > On Sun, Feb 16, 2025 at 11:40 AM Jarek Potiuk <ja...@potiuk.com> wrote: > >> https://github.com/apache/airflow/pull/46800 -> there you go :) . Happy >> reviewing of 1570+ files changed. >> >> BTW. It's way easier to review it with `git diff` than with Github UI :) >> >> On Sun, Feb 16, 2025 at 11:32 AM Jarek Potiuk <ja...@potiuk.com> wrote: >> >>> Naaa. that one is super easy to implement and automate and it's better >>> to have one quick "surgical" cut to rename all of those :), I am about to >>> send PR with all of those changes in a moment. >>> >>> On Sun, Feb 16, 2025 at 11:26 AM Buğra Öztürk <ozturkbugr...@gmail.com> >>> wrote: >>> >>>> Amazing!:) Maybe we can divide and conquer this on structure changes and >>>> pre-commit rather than leaving you with all :) >>>> >>>> On Sun, 16 Feb 2025, 10:45 Jarek Potiuk, <ja...@potiuk.com> wrote: >>>> >>>> > Yeah "unit" seems to be the way to go :) . I will make another giant >>>> PR >>>> > (but this time it should be even easier to review) to convert to it :) >>>> > >>>> > On Sun, Feb 16, 2025 at 10:41 AM Buğra Öztürk < >>>> ozturkbugr...@gmail.com> >>>> > wrote: >>>> > >>>> > > Thanks for all the effort on this! It looks great! >>>> > > - The approach looks great! >>>> > > - Heavy plus one on this. The pre-commit should be one of the ground >>>> > > pillars of any standardisation. Having an extra file structure check >>>> > could >>>> > > be included too on top of `from` to eliminate new providers to >>>> follow the >>>> > > consistent structure. >>>> > > - I like the unit too over provider_tests. It is a great idea to >>>> > eliminate >>>> > > ambiguity. The other parts seem pretty pretty good. >>>> > > >>>> > > Along with the latest improvements, these will be great additions! >>>> > > >>>> > > >>>> > > On Sun, Feb 16, 2025 at 4:15 AM Rahul Vats <rah.sharm...@gmail.com> >>>> > wrote: >>>> > > >>>> > > > Thanks for the detailed explanation and for driving this effort—it >>>> > looks >>>> > > > like a solid improvement. >>>> > > > >>>> > > > 1. >>>> > > > >>>> > > > I love the approach & new structure. I can't think of a better >>>> > > approach. >>>> > > > 2. >>>> > > > >>>> > > > I agree that adding a pre-commit check to enforce consistent >>>> imports >>>> > > is >>>> > > > a good idea. It will help maintain the new structure and >>>> prevent >>>> > > > regressions. >>>> > > > 3. >>>> > > > >>>> > > > For the naming, I’d prefer using unit instead of >>>> provider_tests to >>>> > > keep >>>> > > > it more intuitive and consistent with system and integration. >>>> This >>>> > > makes >>>> > > > the structure self-explanatory and easier for new contributors. >>>> > > > >>>> > > > >>>> > > > Regards, >>>> > > > Rahul Vats >>>> > > > >>>> > > > On Sun, 16 Feb 2025 at 03:50, Jarek Potiuk <ja...@potiuk.com> >>>> wrote: >>>> > > > >>>> > > > > > 2) no real opinion, seems useful. Let’s see if we can do it >>>> with a >>>> > > ruff >>>> > > > > rule first? It _might_ be possible. (It is possible to extend >>>> the >>>> > ruff >>>> > > > > rules on a per subproject basis, check out >>>> task_sdk/pyproject.toml) >>>> > > > > >>>> > > > > I am not sure if we need ruff rule for it, as we have not yet >>>> enabled >>>> > > any >>>> > > > > `ruff` airflow rules in airflow development (it is targeted so >>>> far >>>> > for >>>> > > > > users writing DAGs} >>>> > > > > >>>> > > > > I think it's more "airflow internal" rule not something that >>>> can be >>>> > > used >>>> > > > > for general audience (which is pretty-much what ruff rules are >>>> about) >>>> > > > > >>>> > > > > Unfortunately - the current way ruff works does not allow local >>>> > > > > "plugin" functionality to add new rules that would only be >>>> applicable >>>> > > to >>>> > > > > the current project you are in. In the absence of that feature, >>>> > having >>>> > > a >>>> > > > > python pre-commit seem the next-best thing. >>>> > > > > >>>> > > > > J. >>>> > > > > >>>> > > > > >>>> > > > > On Sat, Feb 15, 2025 at 10:50 PM Ash Berlin-Taylor < >>>> a...@apache.org> >>>> > > > wrote: >>>> > > > > >>>> > > > > > 100%. It’s on my list to tackle very soon, it can’t get put >>>> off >>>> > much >>>> > > > > > longer. >>>> > > > > > >>>> > > > > > I have some thoughts (around back compat mostly) but they >>>> aren’t >>>> > > > > collected >>>> > > > > > yet, and I won’t derail this thread. >>>> > > > > > >>>> > > > > > > On 15 Feb 2025, at 21:43, Jarek Potiuk <ja...@potiuk.com> >>>> wrote: >>>> > > > > > > >>>> > > > > > > cc: @ashb @uranusjr @kaxil -> something for consideration >>>> in our >>>> > > > > > > discussions on how to repackage airlfow soon. I keep on >>>> > explaining >>>> > > > why >>>> > > > > > > running code in airflow.__init__.py is a bad idea and >>>> advocating >>>> > > for >>>> > > > > > > removal of it and replace it with explicit initialization, >>>> yet >>>> > that >>>> > > > > topic >>>> > > > > > > have not been discussed yet, but I will plan to start a >>>> > discussion >>>> > > > > about >>>> > > > > > it >>>> > > > > > > soon once we approach the packaging subject. I am not sure >>>> what's >>>> > > > your >>>> > > > > > > thinking is about this - I know you spent consirderable >>>> amount of >>>> > > > time >>>> > > > > on >>>> > > > > > > doing all the "lazy initalization" dance all over the >>>> places, >>>> > and I >>>> > > > > think >>>> > > > > > > it adds a lot of complexity to our code and only partially >>>> solves >>>> > > the >>>> > > > > > > cicular imports problem. But I know @ashb has very strong >>>> feeling >>>> > > > about >>>> > > > > > > being able to do "from airlfow import Dag" - which more or >>>> less >>>> > > > > requires >>>> > > > > > > all this complexity. I just don't think it's worth it. >>>> > > > > > >>>> > > > > > >>>> > > > > >>>> > > > >>>> > > >>>> > > >>>> > > -- >>>> > > Bugra Ozturk >>>> > > >>>> > >>>> >>>
