The world of development never ceases to surprise me. I am honestly quite surprised by almost universal doubts about using asserts :) - but - that's an interesting learning. For me (coming from old C/C++ days) using asserts was pretty common and useful (and not difficult to decide when to use/not use them).
Doubts seem to be on both - contributors and committers side, it's only me and Ash who seem to see a value in asserts. So while I still have another opinion, I am happy to "disagree but engage" and I will be happy to not only remove the existing asserts but also add pre-commit check to prevent from adding new asserts if we agree as a community that it is better. Unless of course there are other people who have not yet responded on this (apparently controversial) topic :) So I am closing this topic now and open quick voting so that others can quickie give their opinion. J. On Wed, Dec 4, 2019 at 3:45 PM Brian Greene <[email protected]> wrote: > As a dev manager, we generally ban the use of assertions or like > constructs in any language. > > If you, as an engineer, think you should check a value... then check it > and take an intentional path afterwards. This is a known path, it can be > tested (or more likely seen as never tested as it’s so rare a condition). > But then you can decide to remove it. > > This means the code is always the same, dev, test, qa, prod, regardless of > runtime options in the environment, the code that runs is what the > developer intended, and is checking what they intended to check. > > Using assertions, you basically allow external forces to change the > cyclomatic complexity of the code by removing bits of it... this means I > can choose to run the code in a way that no longer meets your intent as a > writer. > > “Well it can’t be that way in prod, only used for testing” is a defense of > sorts, but at some point I think you need to be testing a prod-equivalent > binary/runtime... so if the assertion finds something in testing, we’re > saying it’s ok that other issues, maybe only found in qa, will be > ignored/raise runtime error because the nice assertion error message was > optimized away. > > At this point the dev who put in the assertion doesn’t know that’s what’s > failing in qa, because the assertion is gone. > > I think if you support assertions at all in the codebase then you have to > double your testing - testing once with -o and once without. > > At that point you’ll eventually start to see tests fail with -o, but not > without it... or vice versa. > > As long as the folks starting the interpreter can choose to optionally > remove checks in the code, you have to test the code both ways. > > Maybe not 100% applicable to libraries and utilities, but SW like AF it > seems somewhat relevant. > > My $.02 > > Brian > > > Sent from a device with less than stellar autocorrect > > > On Dec 4, 2019, at 7:47 AM, Kaxil Naik <[email protected]> wrote: > > > > Wouldn't it just make sense to remove asserts and use IF condition so > that > > we don't need to set any rules when to assert and when to not assert. > > > > I don't see a huge benefit of using assert but I can see how it can be > > mis-used (used when it shouldn't not be used) sometimes. > > > > Regards, > > Kaxil > > > >> On Wed, Dec 4, 2019 at 1:17 PM Anton Zayniev <[email protected]> > >> wrote: > >> > >> Ok, my thoughts on assertions.I think carefully used assertions could be > >> occasionally useful for internal checks only. Like you checking all good > >> and raise *your* exception with meaningful message to end user. But in > >> general looks like confusion harm would outweight readability benefit. > >> Just a simple AssertionError would confuse average pythonist as > developer > >> expects (and official docs clearly states that) assertion usage in test > >> environment only. There are alternatives that are as readable as > >> assertion. `assert self.futures, NOT_STARTED_MESSAGE` -> `if not > >> self.futures: raise Error(NOT_STARTED_MESSAGE)` > >> > >> On Wed, 4 Dec 2019 at 09:04, Jarek Potiuk <[email protected]> > >> wrote: > >> > >>> Hey @Anton - I think it would be great if (even if you have examples in > >>> Github) add some summary of your thoughts here. I think we should > >> converge > >>> to one place where we discuss it and it's not helpful to move > back/forth > >>> between different media. > >>> > >>> The discussion gets interesting :). > >>> > >>> One argument I have for using asserts is to indeed treat them as > >>> "debugging-purpose only" - it's actually pretty much the same as type > >>> annotations. Type annotations are stripped out always when the code is > >> run > >>> so we cannot rely on them, yet they are super helpful in catching > >> problems > >>> and MyPy is really good in catching some mis-uses of flexibility of > >> python > >>> typing. And we are already using it. > >>> > >>> I think one of the issues (also mentioned by XD) is the "blur > boundary". > >>> Indeed we have some asserts now (some introduced by me :)) that are not > >>> good for asserts but I think it's one more reason to agree some common > >>> ground here and (if we decide that sometimes assertions are helpful > and > >>> recommended) to have clear rules describing where to use them. > >>> > >>> *Good examples:* > >>> > >>> This is a very good example where asserts are much clearer than > if/throw > >> in > >>> my opinion. It's a programming error if end () is run without start(). > >> And > >>> if asserts are stripped-out we got the "None" exceptions two lines > down. > >>> > >>> def end(self) -> None: > >>> """ > >>> Ends the executor. > >>> :return: > >>> """ > >>> assert self.impl, NOT_STARTED_MESSAGE > >>> assert self.manager, NOT_STARTED_MESSAGE > >>> self.impl.end() > >>> self.manager.shutdown() > >>> > >>> Another good example of decorator for CLI methods (maybe the first > assert > >>> should have a meaningful message though). > >>> > >>> @functools.wraps(f) > >>> def wrapper(*args, **kwargs): > >>> """ > >>> An wrapper for cli functions. It assumes to have Namespace instance > >>> at 1st positional argument > >>> > >>> :param args: Positional argument. It assumes to have Namespace > >> instance > >>> at 1st positional argument > >>> :param kwargs: A passthrough keyword argument > >>> """ > >>> assert args > >>> assert isinstance(args[0], Namespace), \ > >>> "1st positional argument should be argparse.Namespace instance, > >> " \ > >>> "but {}".format(args[0]) > >>> > >>> Another good example: the project_id comes from the decorators and will > >>> always be set to some value even if it is specified as Optional. If it > is > >>> not set in the environment, the object will never be instantiated in > the > >>> first place. > >>> > >>> @_fallback_to_project_id_from_variables > >>> @GoogleCloudBaseHook.fallback_to_default_project_id > >>> def is_job_dataflow_running(self, name: str, variables: Dict, > >>> project_id: Optional[str] = None) -> bool: > >>> """ > >>> Helper method to check if jos is still running in dataflow > >>> > >>> :param name: The name of the job. > >>> :type name: str > >>> :param variables: Variables passed to the job. > >>> :type variables: dict > >>> :param project_id: Optional, the GCP project ID in which to start a > >>> job. > >>> If set to None or missing, the default project_id from the GCP > >>> connection is used. > >>> :return: True if job is running. > >>> :rtype: bool > >>> """ > >>> assert project_id is not None > >>> > >>> > >>> *Bad examples:* > >>> > >>> This depends on configuration so it should not be assert: > >>> > >>> if cluster_address is None: > >>> cluster_address = conf.get('dask', 'cluster_address') > >>> assert cluster_address, 'Please provide a Dask cluster address in > >>> airflow.cfg' > >>> > >>> This also depends on configuration and should be an exception:: > >>> > >>> executor_path = executor_name.split('.') > >>> assert len(executor_path) == 2, f"Executor {executor_name} not > supported: > >>> " \ > >>> f"please specify in format > >>> plugin_module.executor" > >>> > >>> J. > >>> > >>> > >>> On Wed, Dec 4, 2019 at 2:37 AM Deng Xiaodong <[email protected]> > >> wrote: > >>> > >>>> -1 from me for using asserts in Airflow code. > >>>> > >>>> Firstly, as some folks pointed out, asserts are there mainly for > >>> debugging > >>>> purposes. > >>>> > >>>> Second, even though using asserts may bring some benefits in specific > >>>> context, it’s not enough to “break even” comparing with the potential > >>>> confusions (e.g. blur boundary between when to use and when NOT to use > >>>> asserts over raising exceptions). > >>>> > >>>> > >>>> XD > >>>> > >>>> On Wed, Dec 4, 2019 at 06:17 Anton Zayniev <[email protected]> > >>>> wrote: > >>>> > >>>>> Email is not so good for code examples, so I've shared my thoughts > >> here > >>>>> <https://github.com/apache/airflow/pull/6596/files#r353451761>. > >>>>> > >>>>> On Tue, 3 Dec 2019 at 16:53, Jarek Potiuk <[email protected]> > >>>>> 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 < > >>> [email protected]> > >>>>>> 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 <[email protected] > >>> > >>>>> 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 < > >>>>> [email protected]> > >>>>>>>> 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/> > >>>>>> > >>>>> > >>>> > >>> > >>> > >>> -- > >>> > >>> Jarek Potiuk > >>> Polidea <https://www.polidea.com/> | Principal Software Engineer > >>> > >>> M: +48 660 796 129 <+48660796129> > >>> [image: Polidea] <https://www.polidea.com/> > >>> > >> > -- Jarek Potiuk Polidea <https://www.polidea.com/> | Principal Software Engineer M: +48 660 796 129 <+48660796129> [image: Polidea] <https://www.polidea.com/>
