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

Move the try outside the loop when this is possible in Airflow core #33975

Merged
merged 2 commits into from
Sep 3, 2023

Conversation

hussein-awala
Copy link
Member

All the blocks with this pattern

while/for:
    try:
        ...
    except:
        return/break/raise

could be refactored to

try:
    while/for:
        ...
except:
    return/pass/raise

Why?

To make the code more readable and fatser:

import timeit

code1 = """
try:
    for i in range(100000):
        if i == 99999:
            raise Exception("test")
except Exception as e:
    pass
"""

code2 = """
for i in range(100000):
    try:
        if i == 99999:
            raise Exception("test")
    except Exception as e:
        pass
"""

time_taken_code1 = timeit.timeit(stmt=code1, number=1000, globals=globals())
time_taken_code2 = timeit.timeit(stmt=code2, number=1000, globals=globals())

print("Time taken by loop in try:", time_taken_code1)
print("Time taken by try in loop:", time_taken_code2)

and here is the result:

$ python try_except_loop.py 
Time taken by loop in try: 2.8077976499916986
Time taken by try in loop: 3.2557358630001545

^ 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.

raise AirflowException(err)
except ImportError as err:
log.critical("Cannot import %s for API authentication due to: %s", backend, err)
raise AirflowException(err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are at this, I wonder if we should remove this exception wrapping. It doesn’t have any value from what I can tell. Same for the one in __init__.py.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same question because it seems useless. However I prefer to keep this PR changing the syntax without any change in the functionality, I will open some new PR to improve the raised exceptions.

@potiuk potiuk merged commit 8918b43 into apache:main Sep 3, 2023
42 checks passed
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.2 milestone Oct 3, 2023
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 3, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 5, 2023
…33975)

* Move the try outside the loop when this is possible in Airflow core

* Use supress instead of except pass

(cherry picked from commit 8918b43)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:CLI area:Scheduler Scheduler or dag parsing Issues area:Triggerer area:webserver Webserver related Issues type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants