You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/11/30 20:22:31 UTC
[GitHub] [airflow] dstandish opened a new pull request, #28019: Use asserts instead of exceptions
dstandish opened a new pull request, #28019:
URL: https://github.com/apache/airflow/pull/28019
It's a little cleaner, and I'm pretty sure the actual practical import here is just getting mypy to cooperate, so I think the intention is clearer this way too.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1332775055
While I heartily sympathise with you, currently we agreed to not use asserts in the main code. We should only use then in tests.
Initial Discussion here: https://lists.apache.org/thread/h22sodtylksn04tzm1f2n3q8h994j38p
Voting thread here: https://lists.apache.org/thread/tl9f344vwjmzh8pqfgbjkx8rqyy7dp9n
I think by disabling pylint (another vote) we lost check for it, it used to be flagged in pre-commits (and if we are going to keep the rule, we likely should add it).
As you will see, the proponent of using asserts (myself including) were out-voted, and we had no other choice by follow it (I even implemented pylint check for that).
Main reason why we (as community decided to forbid it, was potential confusion about the flows
But - maybe it's time to re-open the discussion ? I am still of an opinion that asserts in cases like you mention are rather useful. As we say - "disagree, but engage" - but if we vote again a decision is different, I am super happy to engage on the side of asserts.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1332924717
> > There is an exception for type checking though, so if these are truly only for mypy...
> > https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#dont-use-asserts-outside-tests
>
> Ah yeah. Forgot about it ... And I have not linked the CONTRIBUTING section 🤦 . Thanks @jedcunningham for that one. Refreshed my memory about it.
Yeah so.... This is for type checking afaict
Is it really necessary to add the if?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #28019:
URL: https://github.com/apache/airflow/pull/28019
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1334608169
> we want to use assert for if TYPE_CHECKING when we want to get rid of mypy warning that None has no property etc
Yes, and by my reading, it appears that's exactly what these asserts are there for. Essentially what's required is that executor.start is called before the executor is used. And, it looks like that's in fact what happens.
It looks like you added these initially [here](https://github.com/apache/airflow/commit/a36cfe049a2c5948b24fde7a878fe19cabede5f7).
Presumably, prior to that change, mypy never complained because the attrs were not defined and annotated in the init method -- they were created in `start`. But in your change you initialized them as None and annotated them, e.g. [here](https://github.com/apache/airflow/blob/a36cfe049a2c5948b24fde7a878fe19cabede5f7/airflow/executors/dask_executor.py#L45). I presume that this is why the asserts had to be added (e.g. [here](https://github.com/apache/airflow/blob/a36cfe049a2c5948b24fde7a878fe19cabede5f7/airflow/executors/dask_executor.py#L66)) -- to get mypy to not complain.
That PR I assume predated any discussions about asserts vs raising. Then when it was decided to go with raising, except in the case of mypy appeasement, these asserts were changed. But, it does seem that these are mypy appeasement and could therefore have been left alone, or put behind a type checking guard, as I've proposed.
In any case, if you see it another way, that's fine and you're welcome to close this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1334451150
OK -- I updated to use the `if TYPE_CHECKING` style
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1334545117
> OK -- I updated to use the `if TYPE_CHECKING` style
I think this is misunderstanding. This "if TYPE_CHECKING" is not something that you should add. There are specific places where we want to use assert for `if TYPE_CHECKING` when we want to get rid of `mypy` warning that None has no property etc. The role of "assert" in tnis case is to "calm down" MyPy so that it does not complain, in places where we **know** None cannot hapen for example.
But in the case above, we actually want raise those messages if something unexpected happens. The way it is done now - those messages will never be printed because if TYPE_CHECKING is only true when MyPy parses the code.
I think - with the current state of our approach - you simply can't replace those if-s wiht asserts.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1335958487
BTW. the if TYPE/assert is indeed much better in this case as it shows intention. The previous implementation (of old mine) tricked the future mine into thinking that it was somewhat possible to get into this flow.
Funny thing is that In a way "just" assert wouldn't be as good as "if'd" one for similar reasons - it would not have clear indication that this assert is there only beacuse of MyPy. But now it's crystal clear.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1332821736
> There is an exception for type checking though, so if these are truly only for mypy...
>
> https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#dont-use-asserts-outside-tests
Ah yeah. Forgot about it ... And I have not linked the CONTRIBUTING section 🤦 . Thanks @jedcunningham for that one. Refreshed my memory about it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1335957076
Ah ok. Now I See. Yeah. I think I indeed added it for `None` mypy case when we had not yet agreed asserts case. Makes perfect sense. I thought it was more "flow" protection, but yeah. This way is better.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #28019:
URL: https://github.com/apache/airflow/pull/28019#issuecomment-1332788056
There is an exception for type checking though, so if these are truly only for mypy...
https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#dont-use-asserts-outside-tests
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #28019: Use asserts instead of exceptions for executor not started
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #28019:
URL: https://github.com/apache/airflow/pull/28019#discussion_r1037587532
##########
airflow/executors/base_executor.py:
##########
@@ -33,8 +33,6 @@
PARALLELISM: int = conf.getint("core", "PARALLELISM")
-NOT_STARTED_MESSAGE = "The executor should be started first!"
-
Review Comment:
maybe we need to maintain this for backcompat but that's basically xkcd1172
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org