We seem to differ. I'd love what other contributors and committers think as
well.
This should never happen because you should start executor right after
instantiating it and the assert message is quite clear about it. It's
actually a constant:
NOT_STARTED_MESSAGE = "The executor should be started first!"
What "assert" command does is it throws AssertionError("The executor should
be started first!") - I think it's quite clear what's going on. There are
about 30 places in executor-related code that might throw the same
exception. And if this line is optimised, few lines below the exception
will be thrown with "this None here have no property such and such).. It is
only triggered if (during development or testing) you made a code
modification that does not start the executor after instantiating the
Executor object. So if this message happens this is not a user problem or
anything that the user can do anything about. It is more of a developer bug
(usually byproduct of some refactoring/adding new feature, etc.). It should
never, ever happen in production code.
It has two purposes:
- aid developers to tell them their refactoring/fix/new features is not
finished yet. Throwing AssertionError for developers is an important
message - that this is not a "user"/"input" problem, but that it is a
developer problem and that it should actually never happen so developer
should immediately look for a bug in their code.
- block MyPy and other type checkers from reporting that the field might be
None. MyPy is smart enough to understand that after such assertion, the
self.futures is never None.
I think it perfectly fits the "assertion" definition and it's much, much
cleaner. And i find using asserts in such cases really useful.
But again - I am also happy to switch to throwing an explicit exception
rather than assertions if majority of contributors and committers will
agree that it's a practice that we agree not to follow.
J.
On Tue, Dec 3, 2019 at 6:07 PM Iuliia Volkova <xnuins...@gmail.com> wrote:
> So, now, in this code I will have no idea that is this, is this error from
> Airflow or somebody forget to remove from master code debug assert? So with
> normal error it will be like this:
>
> *assert self.futures, NOT_STARTED_MESSAGE*
>
> *if not self.futures: *
> * raise AirflowException(NOT_STARTED_MESSAGE)*
>
> second variant: more readable, does not cause any issues with any flags, I
> see in traceback what kind of error is this - some random Apache Airflow or
> maybe ValueError, or maybe TypeError - I have more information as developer
>
> And at the end of the day '*debug*' tool not used in production code.
>
>
> On Tue, Dec 3, 2019 at 7:53 PM Jarek Potiuk <jarek.pot...@polidea.com>
> wrote:
>
> > Just an example of such asserts which IMHO are nicer are here:
> >
> >
> https://github.com/apache/airflow/pull/6596/files#diff-4c0c36f193f2cd65e2b55ba3102c1ba2R38
> > One line assert with message.
> >
> > J.
> >
> >
> >
> > On Tue, Dec 3, 2019 at 5:36 PM Anton Zayniev <anton.zayn...@gmail.com>
> > wrote:
> >
> > > Hi, guys. I'm really surprised about this
> > >
> > > > - (+) asserts look nicer and are more readable than if (something):
> > > > throw Exception()
> > >
> > > I'm pretty sure that all the code I have encountered a way more
> readable
> > > using "if/else" or "try/except". But may be it is just me. Could you
> > > provide an example of code which is better with "assert"?
> > >
> > > - (+) asserts are especially good for cases like None exception - they
> > > > add more developer friendly messages when they will fail a few
> lines
> > > > below
> > > > with (for example) None has no property "dag". But it's ok if
> those
> > > get
> > > > optimised away.
> > >
> > > I think the best way to catch None is to ensure your code would fail
> > > conveniently. Like raising understandable Exception message, if you
> > believe
> > > that should be a point of confusion.
> > >
> > > On Tue, 3 Dec 2019 at 16:22, Iuliia Volkova <xnuins...@gmail.com>
> wrote:
> > >
> > > > Hi everyone, I'm usually not write anything in this mail list, but
> this
> > > > theme something really strange
> > > > Exist offissial doc:
> > > >
> > >
> >
> https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement
> > > >
> > > > and there is a key information: Assert statements are a convenient
> way
> > to
> > > > insert debugging assertions into a program.
> > > >
> > > > *Debugging. * - this is a key propose of asserts keyword.
> > > >
> > > > there is no any type of possible asserts that cannot be done with
> > normal
> > > > Exceptions and Errors types that more explicit and detailed when
> > > 'assert' -
> > > > you have ValueError, TyperError and etc. what kind of problems must
> > > solved
> > > > DEBUG tools in production code that can be easily turned off on
> servers
> > > by
> > > > users?
> > > >
> > > > asserts used in tests and in process of debug code, not in production
> > > >
> > > > On Tue, Dec 3, 2019 at 6:47 PM Jarek Potiuk <
> jarek.pot...@polidea.com>
> > > > wrote:
> > > >
> > > > > We had a few discussions about using asserts in our code. I pasted
> > some
> > > > > links below but wanted to extract a gist of it.
> > > > >
> > > > > Here are the comments summarised:
> > > > >
> > > > > - (+) asserts look nicer and are more readable than if
> > (something):
> > > > > throw Exception()
> > > > > - (-) asserts can be optimized away with -O flag so we should
> not
> > > > based
> > > > > any real logic on having them
> > > > > - (+) asserts are good in cases that can happen in development
> but
> > > > > should "never happen" in reality
> > > > > - (+) asserts are especially good for cases like None exception
> -
> > > they
> > > > > add more developer friendly messages when they will fail a few
> > lines
> > > > > below
> > > > > with (for example) None has no property "dag". But it's ok if
> > those
> > > > get
> > > > > optimised away.
> > > > >
> > > > > We would like to discuss those points in community and have a
> > > community -
> > > > > driven decision on:
> > > > >
> > > > > 1) whether we should use asserts?
> > > > > 2) in which cases
> > > > > 3) in which cases we should NOT use asserts.
> > > > >
> > > > > J.
> > > > >
> > > > > The links here:
> > > > >
> > > > > Slack Discussion:
> > > > >
> > https://apache-airflow.slack.com/archives/CCQ7EGB1P/p1575364664041300
> > > > >
> > > > > Github threads:
> > > > >
> > > > > -
> > https://github.com/apache/airflow/pull/6596#discussion_r352916409
> > > > > -
> > https://github.com/apache/airflow/pull/6596#discussion_r352914727
> > > > > -
> > > > >
> > >
> https://github.com/apache/airflow/pull/3690#pullrequestreview-143376629
> > > > >
> > > > > Stack overflow link for asserts:
> > > > >
> > > > > - https://stackoverflow.com/a/1838411/5691525
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > J.
> > > > >
> > > > > --
> > > > >
> > > > > Jarek Potiuk
> > > > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > > >
> > > > > M: +48 660 796 129 <+48660796129>
> > > > > [image: Polidea] <https://www.polidea.com/>
> > > > >
> > > >
> > > >
> > > > --
> > > > _________
> > > >
> > > > С уважением, Юлия Волкова
> > > > Тел. : +7 (911) 116-71-82
> > > >
> > >
> >
> >
> > --
> >
> > Jarek Potiuk
> > Polidea <https://www.polidea.com/> | Principal Software Engineer
> >
> > M: +48 660 796 129 <+48660796129>
> > [image: Polidea] <https://www.polidea.com/>
> >
>
>
> --
> _________
>
> С уважением, Юлия Волкова
> Тел. : +7 (911) 116-71-82
>
--
Jarek Potiuk
Polidea <https://www.polidea.com/> | Principal Software Engineer
M: +48 660 796 129 <+48660796129>
[image: Polidea] <https://www.polidea.com/>
