Skip to content
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

Make "--preload" flag optional #30910

Closed
1 of 2 tasks
Lorenzo-N opened this issue Apr 27, 2023 · 7 comments · Fixed by #32102
Closed
1 of 2 tasks

Make "--preload" flag optional #30910

Lorenzo-N opened this issue Apr 27, 2023 · 7 comments · Fixed by #32102

Comments

@Lorenzo-N
Copy link

Apache Airflow version

2.5.3

What happened

Hi, I updated airflow from version 2.2.0 to version 2.5.3.
I developed an airflow plugin that adds some REST API by implementing a flask_blueprint and in version 2.2.0 I could develop the plugin using the environment parameter reload_on_plugin_change=True. Updating to v2.5.3 this parameter seems to have no effect anymore, and if I change the plugin the changes are not updated until I restart the webserver.

What you think should happen instead

I expect plugins to be reloaded upon change if the parameter reload_on_plugin_change is set to true.

How to reproduce

Operating System

Extension of official Docker image apache/airflow:2.5.3-python3.10

Versions of Apache Airflow Providers

No response

Deployment

Docker-Compose

Deployment details

No response

Anything else

I investigated the cause of the problem and it seems to be due to the --preload parameter that is added to the gunicorn args. In fact, the workers are restarted when one of the plugin files changes, but the plugins are not updated (in my opinion because the parameter shares the app among all the workers so the plugins are not reloaded). Launching the webserver with the --debug parameter correctly reloads the plugins, however, I would like to use the gunicorn in development to test its correct operation as well.
In my opinion a solution might be not to pass the --preload parameter to the gunicorn if the variable reload_on_plugin_change is set, so that the plugins are reloaded correctly.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@Lorenzo-N Lorenzo-N added area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Apr 27, 2023
@kobethuwis
Copy link
Contributor

I've experienced issues after upgrading my Airflow version as well a while ago. I managed to solve it by using a combination of two changes: #30281 (comment)

@Lorenzo-N
Copy link
Author

Thanks for the reply, but I think I have a different problem since I don't use Kubernetes, I already set reload_on_plugin_change=True in the env and I have the __init__.py file in each plugin folder. Also I have the problem when I change any plugin file with api definitions, not a static file.

@potiuk potiuk removed the needs-triage label for new issues that we didn't triage yet label May 2, 2023
@potiuk potiuk changed the title Reload_on_plugin_change does not work properly Make "--preload" flag optional May 2, 2023
@potiuk potiuk added kind:feature Feature Requests good first issue and removed kind:bug This is a clearly a bug labels May 2, 2023
@potiuk
Copy link
Member

potiuk commented May 2, 2023

Yes. The --preload flag has been added to optimize airlfow loading and maintain integrity of the DB: #27297 and yes, it might break some workflows regarding the development of flask applications like you do.

We are targetting airflow for production use and this is clearly a development feature, so we do not classify it as a bug.

For now I recommend you to just comment this change out while you are developing the API. However i also encourage you (and anyone who my stumble upon this issue) to add a new feature where the --preload flag might be disabled for development case.. A good idea might be to not add the --preload flag when certain env variable is set (we do not want to clutter airflow config with developer options) and add some documenttion in Plugin section of our docs about it.

I'd encourage you also to submit a PR fixing that @Lorenzo-N if you feel like contributing back to airflow is a good idea - otherwise it will wait for someone who will pick it up (I marked it as "good first issue").

@Lorenzo-N
Copy link
Author

Could a simple change like #31012 suffice, reusing the reload_on_plugin_change variable to disable the --preload flag?

@potiuk
Copy link
Member

potiuk commented May 2, 2023

I believe it's not good . Reload on plugin change is a production setting. that allows you to reload generally plugins (but not flask API lie is your case). But I am not 100% sure - of that - someone who is more of an expert there shoud assess the impact of that.

Maybe you can assess that?

@dejadunlap
Copy link

Hi, if no one else has taken it can I take on this issue?

@potiuk
Copy link
Member

potiuk commented Jul 21, 2023

I think it's being implemented in #32102

@potiuk potiuk linked a pull request Jul 21, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants