[GitHub] [airflow] ToxaZ commented on a change in pull request #6596: [AIRFLOW-6004] Untangle Executors class to avoid cyclic imports

[GitBox <gi...@apache.org>]

ToxaZ commented on a change in pull request #6596: [AIRFLOW-6004] Untangle Executors class to avoid cyclic imports
URL: https://github.com/apache/airflow/pull/6596#discussion_r353451761
 
 

 ##########
 File path: airflow/executors/dask_executor.py
 ##########
 @@ -66,20 +63,17 @@ def execute_async(self,
                       command: CommandType,
                       queue: Optional[str] = None,
                       executor_config: Optional[Any] = None) -> None:
-        if not self.futures:
-            raise AirflowException("Executor should be started first.")
+        assert self.futures, NOT_STARTED_MESSAGE
 
 Review comment:
   I'd rather check our options first. How we may avoid assertions.
   
   1. If we need a oneliner: `if not self.futures: raise Error(NOT_STARTED_MESSAGE)`. It is not very elegant but easy to understand.
   
   1. Another option to consider is writing a single checker and call it wether we need it:
   
       ```python
       def _check_started(self):
               if None in (self.client, self.futures):
                   raise AirflowError(NOT_STARTED_MESSAGE)
       ```
   
   1. Or go more pythonic and introduce decorator.
       
       ```python
       def check_futures(fn):
           def wrapped(self):
               if not self.futures:
                   raise AirflowError(NOT_STARTED_MESSAGE)```
           return wrapped
   
       @check_futures
       def execute_async()
           <...>
       ```
   
   1. If we expect that some methods need some time to wait for executor start, [tenacity](https://tenacity.readthedocs.io/en/latest) could be useful:
       
       ```python
       from tenacity import retry
   
       def raise_not_started():
           raise AirflowError(NOT_STARTED_MESSAGE)
   
       @retry(stop=stop_after_attempt(3))
       def execute_async()
           try:
               <...>
           except Exception:
               raise AirflowError((NOT_STARTED_MESSAGE)
       ```
   
   My vote goes to decorator solution -- either 3 or 4. Anyway, I think assertions could be occasionally useful for internal checks only. Like you checking all good and raise _your_ exception with meaningful message to end user. Otherwise it looks like a dirty hack. Just a simple AssertionError would confuse average pythonist as developer expects (and official docs clearly states that) assertion usage in test environment only. Confusion harm would outweight readability benefit.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services