Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Mar 5, 2023

This reverts commit 88ed20a.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:Scheduler including HA (high availability) scheduler area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Mar 5, 2023
@potiuk potiuk requested review from o-nikolas and removed request for o-nikolas March 5, 2023 00:45
@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

Not sure why, but I think ##29609 broken pytest collection in main (looks like a circular import). For some reason it did not happen in the original PR.

We should merge it (and find the cause) as both PRs started to fail with:

    from airflow.executors import celery_executor, celery_kubernetes_executor
airflow/executors/celery_executor.py:40: in <module>
    from celery import Celery, Task, states as celery_states
E   ImportError: cannot import name 'Celery' from 'celery' (/usr/local/lib/python3.7/site-packages/celery/__init__.py)

For example here: https://github.com/apache/airflow/actions/runs/4333507290/jobs/7566768495

@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

@o-nikolas - you will have to redo that one again and we need to carefully look at what was going on with the import.

@potiuk potiuk requested review from Taragolis and eladkal March 5, 2023 00:52
@o-nikolas
Copy link
Contributor

@o-nikolas - you will have to redo that one again and we need to carefully look at what was going on with the import.

Interesting, I'll have to find the way to replicate the error on my end. Both my local testing and the PR didn't show the issue. I'll look on Monday. And thanks for the revert 👍

@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

Yep. Strange. I need approval to merge it though :)

@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

I also added this one #29923 to clearer show when things like that happen (there will be separate job doing the test collection) and also allowing the other jobs in CI to continue even if collection fail.

@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

🙏 :) @o-nikolas

@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

Strange enough it did NOT fail in #29923 ... so I start thinking there could be some intermediate problem.

@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

I think it is a different thing - side-effect of argcomplete released few moments ago (which drags importlb upgrade with it).

@potiuk potiuk marked this pull request as draft March 5, 2023 01:29
@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

Confirmed. This is a regrression in importlib_metadata that will be fixed in upcoming celerly 5.3. Workaround is coming.

@potiuk potiuk closed this Mar 5, 2023
@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

(so no need to take a look @o-nikolas )

@potiuk
Copy link
Member Author

potiuk commented Mar 5, 2023

the right fix is here #29924

@o-nikolas
Copy link
Contributor

Nice, glad this worked out 😃 I was unavailable this weekend and only had a few mins to check this PR on my phone!

@potiuk potiuk deleted the revert-executor-production-decoupling branch March 7, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Scheduler including HA (high availability) scheduler area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants