You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "RNHTTR (via GitHub)" <gi...@apache.org> on 2024/01/03 00:57:03 UTC
[PR] Replace numpy example with practical exercise demonstrating top-level code [airflow]
RNHTTR opened a new pull request, #35097:
URL: https://github.com/apache/airflow/pull/35097
I'm not sure the numpy example is still accurate. I tested out importing numpy locally and it was more or less instantaneous. I think this can cause some confusion implying that no imports should be done at the top level.
<!--
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/
-->
<!-- 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.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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1774724323
FWIW I’ve been wondering if it’s worthwhile to implement some magic in Dagprocessor to automatically move imports inside task functions so people can write DAG files “normally” but receive the function-level import benefits.
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1874970706
Docs building issues :)
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1879020534
Woohoo!
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #35097:
URL: https://github.com/apache/airflow/pull/35097
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1773764486
One thing worth mentioning that even 0.3 s is pretty impactful. All our DAGs are parsed in DAG file process in a separately forked processes - so if such 'pandas' import happens at top level of all DAG files, by default the 0.3 s is an overhead for every dag every 30s - or min-parsing interval.
We already deal with some of that https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html#parsing-pre-import-modules (but only for airflow imports, not for other expensive imports). Also I think it's worth keeping import for another case, explicitly in. I had seen quite a few examples where "organisation-level" imports were doing a lot of things - and it's been hidden from DAG developer (for example I saw pulling configuration from via expensive API call happening there).
People often don't realize that just running `import something` might be very expensive, so teaching people that it might happen (and that it heavily impacts DAG parsin) is an important function of that page.
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1773816055
Yeah but it does matter and I think we should explain it to educate the users. Similarly as the user who asked the question about PEP8 conflicting with our advice, they might not realize they are heavily impacting performance of scheduler/DAG file processor.
Over the last few years or so @uranusjr spend enormous effort on moving a number of imports of ours to make sure airflow imports faster, shaving 100s of milliseconds sometimes precisely because it **actually** matters.
I also added answer to the same question in Slack of our (seems that the author have been asking in multiple places)
https://apache-airflow.slack.com/archives/CCQ7EGB1P/p1697819766612259
> Things evolve and PEP8 written 5th of July 2001 have not foreseen many of things that happened during those 22 years
> Look at this Ruff rule for example: https://docs.astral.sh/ruff/rules/banned-module-level-imports/
> And Ruff is the "de-facto" linter for Python that took Python world by the storm and provides way more linting capabilities than PEP8 did
> So if you compare the two, you have the choice of following 22 years old rule for that or something that became the de-facto standard over the last 2 years or so. Up to you, but our advice is to follow the modern trends.
So while PEP8 - years ago was good advice, it seems that Python community recognised that and "top level imports for expensive modules" is the recommended practice (and explicitly showing how to do it makes sense IMHO).
The advice from Ruff authors mentions `torch` and `tensorflow` - both being expensive and popular in data science world.
So why don't we simply clarify it, for example this way - if we would like to give really precise advice to our users (I took it from some of our example DAGs)
```python
# It's ok to import modules that are not expensive to load at top-level of DAG file
import random
import pendulum
# Expensive imports should be avoided as top level imports because DAG is imported frequently
# even if it does not follow PEP8 advice (PEP8 have not foreseen that certain imports will be very expensive)
# DON'T DO THAT - import them locally instead (see below)
#
# import pandas
# import torch
# import tensorflow
#
@task()
def do_stuff_with_pandas_and_torch():
import pandas
import torch
do_some_operations_using_these
@task()
def do_stuff_with_tensorflow():
import tensorflow
do_some_operations_using_these
.....
Would you find this confusing ? I think it would be rather help our users with decisions how to write the DAGs.
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "RNHTTR (via GitHub)" <gi...@apache.org>.
RNHTTR commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1773804072
I did add this blurb about imports:
> Note that import statements also count as top-level code. So, if you have an import statement that takes a long time or the imported module itself executes code at the top-level, that too can impact the performance of the scheduler.
Even the pandas example feels like it'd be a relatively small optimization compared to the kind of problems that more significant top level code can cause, and IMO including that example could cause more confusion than good for folks who are newer to Airflow. For example, this PR was inspired by [this stackoverflow question](https://stackoverflow.com/q/77332542/7004653).
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1773761513
I like the idea of mentioning and being explicit about expensive APIs but I think there is a value in mentioning the imports, because not many people are aware how big of an impact such imports might have.
I believe `numpy` was indeed not a good example. It does import slowly for the first time but mostly it's because it has to load a lot of C (.so) libraries into memory, numpy is just a thin wrapper around mostly C code, so once those .so libraries are loaded, the import will be fast. Generally anything < 0.2 s seems instanteneous (and numpy imports faster than that).
I suggest to add the "expensive" operation as well, but also keep "slow import" example in - but replacing it with `pandas` to show much bigger effect it might have. Pandas is written mostly in Python (and uses numpy under the hood among others) and it is notoriously known from being slow to import as it imports ~700 python files (and that all after `__pycache__` and `.pyc` bytecode files have been computed and numpy shared .so libraries loaded in memory).
Some experiments:
It takes some 0.3 - 0.4 s to import on my MacOS:
```
python -c 'import pandas' 0.46s user 1.91s system 649% cpu 0.364 total
[jarek:~] [airflow-3.11] % time python -c 'import pandas'
python -c 'import pandas' 0.65s user 1.73s system 647% cpu 0.367 total
[jarek:~] [airflow-3.11] % time python -c 'import pandas'
python -c 'import pandas' 0.72s user 1.46s system 658% cpu 0.331 total
[jarek:~] [airflow-3.11] % time python -c 'import pandas'
python -c 'import pandas' 0.45s user 1.69s system 628% cpu 0.341 total
[jarek:~] [airflow-3.11] % time python -c 'import pandas'
python -c 'import pandas' 1.08s user 1.34s system 562% cpu 0.430 total
```
So ~ 0.5 s on my MacOS.
And around ~ 0.3 s in my docker container:
```
root@cbc7d85dfa99:/opt/airflow# time python -c 'import pandas'
real 0m0.323s
user 0m0.781s
sys 0m0.066s
root@cbc7d85dfa99:/opt/airflow# time python -c 'import pandas'
real 0m0.334s
user 0m0.780s
sys 0m0.079s
root@cbc7d85dfa99:/opt/airflow# time python -c 'import pandas'
real 0m0.291s
user 0m0.760s
sys 0m0.056s
root@cbc7d85dfa99:/opt/airflow# time python -c 'import pandas'
real 0m0.291s
user 0m0.742s
sys 0m0.075s
root@cbc7d85dfa99:/opt/airflow# time python -c 'import pandas'
real 0m0.284s
user 0m0.744s
sys 0m0.057s
```
Importing Pandas results in opening around 750 files:
```
strace python -c 'import pandas' 2>&1 | grep openat | wc
750 3972 96013
```
The same exercise for numpy shows that it is much faster in container (~0.1s) and opens far less number of files:
```
root@cbc7d85dfa99:/opt/airflow# time python -c 'import numpy'
real 0m0.105s
user 0m0.342s
sys 0m0.028s
root@cbc7d85dfa99:/opt/airflow# time python -c 'import numpy'
real 0m0.141s
user 0m0.571s
sys 0m0.026s
root@cbc7d85dfa99:/opt/airflow# time python -c 'import numpy'
real 0m0.126s
user 0m0.352s
sys 0m0.038s
root@cbc7d85dfa99:/opt/airflow# time python -c 'import numpy'
real 0m0.122s
user 0m0.341s
sys 0m0.044s
```
Opened files:
```
strace python -c 'import numpy' 2>&1 | grep openat | wc
291 1593 35597
```
Fragment of the strace for pandas - showing that it imports a lot of code.
```
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/computation", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/computation/__pycache__/expressions.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/computation/__pycache__/check.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/ops/__pycache__/missing.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/ops/__pycache__/dispatch.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/ops/__pycache__/invalid.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/ops/__pycache__/common.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/ops/__pycache__/docstrings.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/ops/__pycache__/mask_ops.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pandas/core/arrays/__pycache__/_arrow_string_mixins.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pyarrow/__pycache__/compute.cpython-311.pyc", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/local/lib/python3.11/site-packages/pyarrow/_compute.cpython-311-aarch64-linux-gnu.so", O_RDONLY|O_CLOEXEC) = 3
```
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1846298315
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #35097: Replace numpy example with practical exercise demonstrating top-level code
URL: https://github.com/apache/airflow/pull/35097
--
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] Replace numpy example with practical exercise demonstrating top-level code [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35097:
URL: https://github.com/apache/airflow/pull/35097#issuecomment-1774812370
> FWIW I’ve been wondering if it’s worthwhile to implement some magic in Dagprocessor to automatically move imports inside task functions so people can write DAG files “normally” but receive the function-level import benefits.
That would be rather super-magical if we manage to pull it off IMHO. I thin the **most** we should do is to detect and warn such expensive imports (which BTW. I think is a good idea) - but manipulating the sources or bytecode of the DAG files written by the user is very dangerous, Not only it will change line numbers for debugging but there are a number of edge cases - for example user might **really** have a good reason to import even expensive imports at module (top) level.
There are also all the "transition" cases - DAG imports a utility code that imports tensorflow. This is equally expensive (the utility import is). Should we move the whole utility import to inside a task? Which task? Maybe the utility method also initializes some code that might be needed for all tasks (like setting variables needed to authenticate inside the organisation) etc. etc. We really do not want to get involved in those.
But we **could**potentially warn if we see an import that we consider as "expensive" after DAG is parsed and warn the user. That would be very simple to impement and nice feature I think. We could even likely consider measuring (by some smarts or monkeypatching of python stdlib code I think) the time it takes to do imports and automatically flag imports that take (say) > 0.2s. Why not ?
--
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