You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "imurchie (via GitHub)" <gi...@apache.org> on 2024/03/05 18:46:14 UTC
[PR] fix: do not use os.fork on MacOS [airflow]
imurchie opened a new pull request, #37908:
URL: https://github.com/apache/airflow/pull/37908
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
In MacOS using `os.fork` is not threadsafe, and is also unsafe for use with higher level systems APIs (see [os.fork()](https://docs.python.org/3/library/os.html#os.fork)).
Airflow already uses `subprocess.Popen` in environments without `os.fork`, so force it to do so for MacOS as well.
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
Re: [PR] fix: do not use os.fork on MacOS [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37908:
URL: https://github.com/apache/airflow/pull/37908#issuecomment-1979983139
> What would be the huge impact? A single load-time check seems unlikely to cause any noticeable performance change.
NOT forking when you can is the huge impact. Executing every single task takes few seconds more, because you need to spawn new interpreter, import 100s of modules and initialize them. DAG file processing has to do exactly the same (spawn new interpreter and import all that is needed to parse EVERY SINGLE DAG FILE - EVERY TIME IT DOES IT - i.e. every 30 seconds by default. Forks also use copy-on-write memory, which means that hen you for such pre-intialized processes, they use nearly 0 extra memory - even if you bave many of them, because most memory initialized in the parent process is not modified and shared between all the child processes.
This is a huge performance hit. For example if you run your airlfow on your Mac it means that it will be almost constantly (due to DAG file processor) using a lot of CPU and memory for just scannind the DAG folder. A LOT more CPU and A LOT more CPU.
Likely when you run airflow in your local virtualenv time of your battery in your laptop will be at least halved if not decreased by 60%. Not mentioning the environmental effect.
It's not "one switch check", it's the fact that you will stop using forking on MacOS during development that is problematic. You have to be very, very careful, when you want to propose such a change, especially knowing that you will impact the development environment, ergonomy and quality of development for many of the contributiors - my "rough" assesment is that at least 50%-60% of our contributors use mac, and likely 80-90% of maintainers.
You basically propose to decrease their quality-of-work significantly be **just** disabling forking on MacOS
--
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
Re: [PR] fix: do not use os.fork on MacOS [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37908:
URL: https://github.com/apache/airflow/pull/37908#issuecomment-2071156206
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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
Re: [PR] fix: do not use os.fork on MacOS [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #37908:
URL: https://github.com/apache/airflow/pull/37908#issuecomment-1979449467
> os.fork is not threadsafe,
🤔 🤔 It has side effects with some system API e.g. proxies, which to be honest is known issue, and if SIGSEGV happen in worker it will reports to end users.
--
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
Re: [PR] fix: do not use os.fork on MacOS [airflow]
Posted by "imurchie (via GitHub)" <gi...@apache.org>.
imurchie commented on PR #37908:
URL: https://github.com/apache/airflow/pull/37908#issuecomment-1979943479
> It will have a huge impact on speed of testing for development. And MacOS is anyhow supported only for development.
What would be the huge impact? A single load-time check seems unlikely to cause any noticeable performance change.
--
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
Re: [PR] fix: do not use os.fork on MacOS [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37908:
URL: https://github.com/apache/airflow/pull/37908#issuecomment-1979849611
It will have a huge impact on speed of testing for development. And MacOS is anyhow supported only for development.
My suggestion is to add configuration value to disable it, and possibly make a warning (and also add explanation how you can turn that flag off) in the message we explain SIGSEGV
I personally NEVER experience thread -unsfeness or SIGSEGV during my development on MacOS. It would be a pity to heavily impair speed of running Airflow for the really rare event when it will cause problems.
--
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
Re: [PR] fix: do not use os.fork on MacOS [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #37908: fix: do not use os.fork on MacOS
URL: https://github.com/apache/airflow/pull/37908
--
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