airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jarek Potiuk <Jarek.Pot...@polidea.com>
Subject Re: [2.0 spring cleaning] Changes in import paths
Date Mon, 17 Jun 2019 11:37:58 GMT
Yeah. I think it's worth even if it's a bit of boilerplate code. I think we
might be a bit more friendly and welcoming to new/accidental users who just
want to make a small PR and are not regular users. Those people will often
use IDEs (and Pycharm is by far the most popular in python world).
Similarly to AIP-7 I think if those people have lower barrier entry and
better guidance, we will only all benefit from that :).

J.

On Tue, Jun 11, 2019 at 8:53 PM Kamil Breguła <kamil.bregula@polidea.com>
wrote:

> If I added a warning in the method that it calls immediately, IDE
> still could not detect everything correctly. The user is not able to
> see the error message in the IDE. The warning appears in the wrong
> place.
> https://i.imgur.com/CSqP1wU.png
>
> Adding a new import allows you to avoid some IDE warnings, but the
> problem remains that this module is not detected as deprecated
> https://i.imgur.com/CSqP1wU.png
> .
>
> On Thu, Jun 6, 2019 at 11:32 AM Ash Berlin-Taylor <ash@apache.org> wrote:
> >
> > I hate IDEs :(
> >
> > What does the IDE show if you package up the deprecation warning in some
> way in your solution (i.e. the warning.warn isn't directly in the module
> source)? Alternatively adding `from solution2.new import *` into
> solution2.deprecated may help the IDE?
> >
> > -a
> >
> > > On 5 Jun 2019, at 17:41, Kamil Breguła <kamil.bregula@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 <ash@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.Potiuk@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.bregula@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.Potiuk@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.bregula@polidea.com>
> > >>>>> wrote:
> > >>>>>
> > >>>>>> Inline comment
> > >>>>>>
> > >>>>>> On Fri, Apr 19, 2019 at 12:23 AM Arthur Wiedmer <
> > >>>>> arthur.wiedmer@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.bregula@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.potiuk@polidea.com
> > >>>>>
> > >>>>
> > >>>>
> > >>>> --
> > >>>>
> > >>>> Kamil Breguła
> > >>>> Polidea <https://www.polidea.com/> | Software Engineer
> > >>>>
> > >>>> M: +48 505 458 451 <+48505458451>
> > >>>> E: kamil.bregula@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.potiuk@polidea.com
> > >>
> > >>
> > >
> > > --
> > >
> > > Kamil Breguła
> > > Polidea <https://www.polidea.com/> | Software Engineer
> > >
> > > M: +48 505 458 451 <+48505458451>
> > > E: kamil.bregula@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>
[image: Polidea] <https://www.polidea.com/>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message