Hello community, Here is my take on this:
Case #1: Solution B, but I would like for us to consider putting incubating modules in a separate repository instead of vendoring them with Airflow itself. There are pros and cons to this approach. On one hand, I think that we need to take responsibility for all code in the main repo, and this would help us focus our efforts on high-quality, well-tested packages. However, this would have the effect that incubating packages would see less usage, and therefore might develop more slowly. Case #2: Solution A. Case #3: Solution C, however I do not see it as incompatible with Case #1 Solution B. Case #4: Solution B, but I think that we could consider namespacing incubating packages if solution B is retained for Case #1. Case #5: Solution B. When in doubt, I would rather err on the side of explicitness. Case #6: Agreed that we need to be consistent in naming. Help from CSPs would be very good. Case #7: No opinion. Regards, Philippe On Wed, Jun 5, 2019 at 12:42 PM Kamil Breguła <kamil.breg...@polidea.com> wrote: > I have done a deep investigation of this issue. Please, look at the > beginning of chapter "Case #7". > > I prepared a repository with simple demo and also shared a screenshot from > my IDE. > Photo: https://imgur.com/a/mRaWpQm > Repo: https://github.com/mik-laj/airflow-deprecation-sample > Today I looked again at this project. I recently updated the IDE to the > latest version - IntelliJ IDEA 2019.1.2 (Ultimate Edition). The issue still > exists. > I made additional photos today: https://imgur.com/a/4fcmkVX > > IDE uses statistical analysis of the code and gets lost in external > libraries. > > If you have questions, I will be happy to give you further explanations. > > On Wed, Jun 5, 2019 at 4:30 PM Ash Berlin-Taylor <a...@apache.org> wrote: > > > > Case 7: Zope deprecation or not > > > *B*: I prefer to use Kamil's in-house hack than zope, but I feel we > could > > > improve it slightly to remove a bit of boilerplate - it looks like the > > > warning.warn() message should be very similar in all deprecation cases > > and > > > possibly we can define it in one place (providing that > DepracationWarning > > > will continue to work well with IDEs) > > > > At this point wouldn't or home-rolled solution be very similar to > > zope.deprecation (which is already a dep of Airflow)? > > > > Also are we sure that zope.deprecation isn't supported by an IDE? It > still > > issues a warning.warn. (The example docs in zope.deprecation show messing > > about with sys.modules but that isn't needed.) > > > > https://gist.github.com/ashb/327a79d7c6c1f5a2344cd1812a8ee92d shows a > > formatted example: > > > > ``` > > #Place this as airflow/foo.py > > import zope.deprecation > > zope.deprecation.moved('airflow.settings', 'version 2') > > ``` > > > > And then when you import that module you get: > > > > ``` > > /Users/ash/.virtualenvs/airflow/bin/ipython:1: DeprecationWarning: > > airflow.foo has moved to airflow.settings. Import of airflow.foo will > > become unsupported in version 2 > > #!/Users/ash/.virtualenvs/airflow/bin/python3.7 > > > > ``` > > > > That should work fine with IDE? > > > > > > > On 5 Jun 2019, at 14:56, Jarek Potiuk <jarek.pot...@polidea.com> > wrote: > > > > > > I think it's very important to discuss this and choose the right > approach > > > and follow up quickly. This is very important to introduce some sanity > in > > > the structure of 2.0.0. > > > I propose that anyone with a strong opinion voices his or her concerns > > here > > > and then maybe a week from now we can get to a consensus and vote on > the > > > direction we take? > > > > > > My preferences (but I can be convinced to change some of my choices) > are: > > > > > > Case 1: (contrib vs not) > > > *B* (move all that are "well" tested): I think having "incubator" kind > of > > > approach for not super tested/documented but still usable code is > useful. > > > Though we would have to establish some objective criteria and describe > a > > > "growing from incubation" process. > > > > > > Case 2 airflow.operators.foo_operator.FooOperator could become > > > *A:* airflow.operators.foo.FooOperator > > > > > > Case 3: > airflow.contrib.operators.gcp_bigtable_operator.BigTableOperator > > > could become > > > *C*: airflow.integration.gcp.operators.bigtable.BigTableOperator > > > > > > Case 4: (separate packages/namesapces) > > > *B:* no namespaces - i think we are very far from modularisation to > > happen > > > (and how) and it might get never implemented so there are no real > > benefits > > > I see now. > > > > > > Case 5: "Operator" suffix on class names > > > *B*: I prefer to keep B as this will be much easier when you search for > > > class using your IDE. > > > > > > Case 6: "Isolated" renames > > > *Rename*: We should align names of those operators. > > > > > > Case 7: Zope deprecation or not > > > *B*: I prefer to use Kamil's in-house hack than zope, but I feel we > could > > > improve it slightly to remove a bit of boilerplate - it looks like the > > > warning.warn() message should be very similar in all deprecation cases > > and > > > possibly we can define it in one place (providing that > DepracationWarning > > > will continue to work well with IDEs) > > > > > > J, > > > > > > On Tue, Jun 4, 2019 at 2:11 PM Kamil Breguła < > kamil.breg...@polidea.com> > > > wrote: > > > > > >> I created AIP-21 > > >> > > >> > > > https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths > > >> This is the same document as above, I just deleted my opinions > (summary > > >> sections) and work plan. > > >> > > >> > > >> On Fri, Apr 19, 2019 at 2:18 PM Jarek Potiuk < > jarek.pot...@polidea.com> > > >> wrote: > > >> > > >>> Thanks Kamil! Excellent analysis! > > >>> > > >>> My preferences: > > >>> > > >>> Case 1: (contrib vs not) > > >>> B (move all that are "well" tested): I think having "incubator" kind > of > > >>> approach for not super tested/documented but still usable code is > > useful. > > >>> Though we would have to establish some objective criteria and > describe > > a > > >>> "growing from incubation" process. > > >>> > > >>> Case 2 airflow.operators.foo_operator.FooOperator could become > > >>> A: airflow.operators.foo.FooOperator > > >>> > > >>> Case 3: > > airflow.contrib.operators.gcp_bigtable_operator.BigTableOperator > > >>> could become > > >>> C: airflow.integration.gcp.operators.bigtable.BigTableOperator > > >>> > > >>> Case 4: (separate packages/namesapces) > > >>> B: no namespaces - i think we are very far from modularisation to > > happen > > >>> (and how) and it might get never implemented so there are no real > > >> benefits > > >>> I see now. > > >>> > > >>> Case 5: "Operator" suffix on class names > > >>> B: I prefer to keep B as this will be much easier when you search for > > >> class > > >>> using your IDE. > > >>> > > >>> Case 7: Zope deprecation or not > > >>> B: I prefer to use Kamil's in-house hack than zope, but I feel we > could > > >>> improve it slightly to remove a bit of boilerplate - it looks like > the > > >>> warning.warn() message should be very similar in all deprecation > cases > > >> and > > >>> possibly we can define it in one place (providing that > > DepracationWarning > > >>> will continue to work well with IDEs) > > >>> > > >>> > > >>> > > >>> > > >>> On Fri, Apr 19, 2019 at 12:36 AM Kamil Breguła < > > >> kamil.breg...@polidea.com> > > >>> wrote: > > >>> > > >>>> Inline comment > > >>>> > > >>>> On Fri, Apr 19, 2019 at 12:23 AM Arthur Wiedmer < > > >>> arthur.wied...@gmail.com> > > >>>> wrote: > > >>>> > > >>>>> > > >>>>>>>>> Case #5 Are we talking about the class name or the file name? > > >>> The > > >>>>>> class > > >>>>>>>>> name is fine to me, but we could remove _operator from the > > >> file > > >>>>> name. > > >>>>>>>>> > > >>>>>>>> > > >>>>>>>> Case #2 describes the change of file names > > >>>>>>>> Case #5 describes the change of class names > > >>>>>>>> > > >>>>>>>> I added examples to two cases to better describe the changes. > > >>>>>>>> > > >>>>>>> > > >>>>>>> Thank you. I guess I see it for file names, but I am wondering > > >>> about > > >>>>> the > > >>>>>>> operators and name collisions. > > >>>>>>> Say I need both the HiveOperator and I inline a custom operator > > >> for > > >>>>>> which I > > >>>>>>> need a hive hook. > > >>>>>>> # Would you recommmend the following? Or does case #5 only apply > > >> to > > >>>>>>> Operators? > > >>>>>>> from airflow.operators.hive import Hive as HiveOperator > > >>>>>>> from airfow.hooks.hive import Hive as HiveHook > > >>>>>>> > > >>>>>>> I was just thinking it might be nice to be able to import what I > > >>> need > > >>>>>>> without renaming and not worry too much about names shadowing > > >>> others. > > >>>>>>> Especially if I am new-ish to Apache Airflow. > > >>>>>>> > > >>>>>>> > > >>>>>> I think that the operator should describe the behavior(verb), hook > > >>>> should > > >>>>>> describe a service(noun). These are other parts of speech. > > >>>>>> In my opinion, HiveOperator should be named HiveExecuteQuery. Hook > > >>> will > > >>>>> be > > >>>>>> called Hive. Then there will be no name conflicts. > > >>>>>> > > >>>>> > > >>>>> But we will still provide the backwards compatibility for a while > > >> with > > >>>>> aliases to the old names, correct? > > >>>>> > > >>>> > > >>>> Yes. It is possible to provide backward compatibility also in this > > >> case. > > >>>> > > >>>> > > >>>> -- > > >>>> > > >>>> Kamil Breguła > > >>>> Polidea <https://www.polidea.com/> | Software Engineer > > >>>> > > >>>> M: +48 505 458 451 <+48505458451> > > >>>> E: kamil.breg...@polidea.com > > >>>> [image: Polidea] <https://www.polidea.com/> > > >>>> > > >>>> We create human & business stories through technology. > > >>>> Check out our projects! <https://www.polidea.com/our-work> > > >>>> [image: Github] <https://github.com/Polidea> [image: Facebook] > > >>>> <https://www.facebook.com/Polidea.Software> [image: Twitter] > > >>>> <https://twitter.com/polidea> [image: Linkedin] > > >>>> <https://www.linkedin.com/company/polidea> [image: Instagram] > > >>>> <https://instagram.com/polidea> [image: Behance] > > >>>> <https://www.behance.net/polidea> > > >>>> > > >>> > > >>> > > >>> -- > > >>> > > >>> Jarek Potiuk > > >>> Polidea <https://www.polidea.com/> | Principal Software Engineer > > >>> > > >>> M: +48 660 796 129 <+48660796129> > > >>> E: jarek.pot...@polidea.com > > >>> > > >> > > >> > > >> -- > > >> > > >> Kamil Breguła > > >> Polidea <https://www.polidea.com/> | Software Engineer > > >> > > >> M: +48 505 458 451 <+48505458451> > > >> E: kamil.breg...@polidea.com > > >> [image: Polidea] <https://www.polidea.com/> > > >> > > >> We create human & business stories through technology. > > >> Check out our projects! <https://www.polidea.com/our-work> > > >> [image: Github] <https://github.com/Polidea> [image: Facebook] > > >> <https://www.facebook.com/Polidea.Software> [image: Twitter] > > >> <https://twitter.com/polidea> [image: Linkedin] > > >> <https://www.linkedin.com/company/polidea> [image: Instagram] > > >> <https://instagram.com/polidea> [image: Behance] > > >> <https://www.behance.net/polidea> > > >> > > > > > > > > > -- > > > > > > Jarek Potiuk > > > Polidea <https://www.polidea.com/> | Principal Software Engineer > > > > > > M: +48 660 796 129 <+48660796129> > > > E: jarek.pot...@polidea.com > > > > > > -- > > Kamil Breguła > Polidea <https://www.polidea.com/> | Software Engineer > > M: +48 505 458 451 <+48505458451> > E: kamil.breg...@polidea.com > [image: Polidea] <https://www.polidea.com/> > > We create human & business stories through technology. > Check out our projects! <https://www.polidea.com/our-work> > [image: Github] <https://github.com/Polidea> [image: Facebook] > <https://www.facebook.com/Polidea.Software> [image: Twitter] > <https://twitter.com/polidea> [image: Linkedin] > <https://www.linkedin.com/company/polidea> [image: Instagram] > <https://instagram.com/polidea> [image: Behance] > <https://www.behance.net/polidea> >
