Skip to content

Conversation

@o-nikolas
Copy link
Contributor

@o-nikolas o-nikolas commented Feb 19, 2023

Previously the views code was hardcoded to look out for the SequentialExecutor and warn to not use it in production. This change makes that generalized to any non-production executor.

Some screenshots below:

Screenshot from 2023-03-03 12-37-51
Screenshot from 2023-03-03 12-36-03


^ 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 Feb 19, 2023
@dimberman
Copy link
Contributor

@o-nikolas looks like there are a few failing tests. Once these are fixed would be glad to merge.

@o-nikolas o-nikolas force-pushed the onikolas/aip-51/ui_decoupling branch 2 times, most recently from fab2657 to 947e546 Compare February 21, 2023 16:29
@potiuk
Copy link
Member

potiuk commented Feb 25, 2023

I think it needs rebase.

@o-nikolas
Copy link
Contributor Author

Interesting development:
In this PR I add the notion of is_production to the executor interface, to represent whether or not an executor is meant to be used in production code (executors like DebugExecutor and SequentialExecutor, for example, are not intended for production use).
But the only usage was in the /run endpoint and associated dags.html and @bbovenzi has removed that endpoint entirely in #29706
So shall we keep this PR which adds an attribute to the Executor interface which is now not used? Or scrap it and wait until we find another use case for it? WDYT @potiuk & @dimberman?

@o-nikolas
Copy link
Contributor Author

Thoughts on the above anyone?

@uranusjr
Copy link
Member

uranusjr commented Mar 3, 2023

Scraping it sounds reasonable to me

@potiuk
Copy link
Member

potiuk commented Mar 3, 2023

Yep. Scrap it @o-nikolas

@o-nikolas
Copy link
Contributor Author

Thanks for the feedback! I'll scrap it for now 👍

@o-nikolas o-nikolas closed this Mar 3, 2023
@o-nikolas o-nikolas reopened this Mar 3, 2023
@o-nikolas
Copy link
Contributor Author

Sorry for the confusion, but this code is still needed after all. I thought things had changed in a way that they haven't. I'll update this PR today

Previously the views code was hardcoded to look out for the
SequentialExecutor and warn to not use it in production. This change
makes that generalized to any non-production executor
@o-nikolas o-nikolas force-pushed the onikolas/aip-51/ui_decoupling branch from 947e546 to 79d36d0 Compare March 3, 2023 19:18
@o-nikolas
Copy link
Contributor Author

@potiuk @dimberman @uranusjr

Sorry again for the confusion, but this PR is ready again for review 🙏

@potiuk
Copy link
Member

potiuk commented Mar 4, 2023

LGTM

@potiuk potiuk merged commit 88ed20a into apache:main Mar 4, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 5, 2023
@pierrejeambrun pierrejeambrun added AIP-51 AIP-51: Remove executor coupling from Core changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) labels Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-51 AIP-51: Remove executor coupling from Core area:Scheduler including HA (high availability) scheduler area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants