-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Handle task-sdk connection retrieval import error in BaseHook #54692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle task-sdk connection retrieval import error in BaseHook #54692
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that will be fixed in final 3.1 (@amoghrajesh ?) - but for now it's good as it is.
|
Following the comment of @amoghrajesh -> this actualy should be fixed by installing Question @seanghaeli -> is there something "special" you do to get the environment for system tests? |
I'm curious about this, we're certainly using the task API and we're not doing anything special beyond running the breeze system tests commands.
This is interesting, the image we use for most test workers is either breeze itself (in the local case) or the apache airflow public image from dockerhub. If task-sdk isn't installed in either we can be sure to do that, but shouldn't it be there already @potiuk? |
|
@o-nikolas yes it should. Could either of you show me some of the failures so that we can handle that better if needed? But long term (3.1+), task SDK should be the way to do these things. |
Task SDK is not installed in Airflow 2 - maybe you are using those when running system tests on it? |
We're running on main (so effectively 3.1) this is how this recent change affected our builds |
|
@amoghrajesh the end of the stacktrace I was seeing is: This PR provides the workaround so that the testing infrastructure can handle the thrown error and then fall back to default credentials. |
|
@amoghrajesh Re:
Do you need anything further from @seanghaeli ? |
|
Hey @amoghrajesh We're seeing this in our Kubernetes system tests as well, when run in deferrable mode specifically. Here is a traceback: I would prefer to not put another catch-log-ignore in the cncf hook like we did here for the amazon provider (I'd preferably like to remove that one also). Is it possible for you to have a look at this one? |
|
I've done a lot of deep diving and I think I've identified what's happening here. So the above exception is actually coming from the reentry point when we're returning from the triggerer. Just before that exception in the logs we see: And I see successful communication with task API comms above that. So it's actually within the Triggerer scope that we're failing to communicate with task api to get connection information. This is because in the dag.test() world, we run triggerers inline here and we do not setup the inprocess supervisor comms like we do for regular task execution in dag.test() here. So I believe we need to update the inline execution of triggers in dag.test() to setup a supervisor comms. I have a draft PR here with what I think that might look like. But likely needs refinement: #55236 |
When these lines were merged from #54083:
https://github.com/apache/airflow/pull/54083/files/5f2f2d8c95dfc4bf9bc98c7f9c3bb97f34062da6#diff-acfa34467241916c1189174e98f87a4094e8cbf58d391fa084e6afb2594decbcL61-L71
The Amazon provider system tests all started failing because the testing infrastructure does not use task-sdk. An ImportError is thrown when the testing infrastructure tries to retrieve the connection via task-sdk. In this PR, we handle this import error and the testing infrastructure then falls back to the default connection, which is the standard way the tests are run. With this change, the tests work.