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/> >>> >>
