Skip to content

Conversation

@yati-sagade
Copy link
Contributor

We capture the standard output and error streams so that they're handled
by the configured logger. However, sometimes, when developing dags or
Airflow code itself, it is useful to put pdb breakpoints in code
triggered using an airflow run. Such a flow would of course require
not redirecting the output and error streams to the logger.

This patch enables that by adding a flag to the airflow run
subcommand. Note that this does not require --local.

Make sure you have checked all steps below.

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:

We capture the standard output and error streams so that they're handled
by the configured logger. However, sometimes, when developing dags or
Airflow code itself, it is useful to put pdb breakpoints in code
triggered using an airflow run. Such a flow would of course require
not redirecting the output and error streams to the logger.

This patch enables that by adding a flag to the airflow run
subcommand. Note that this does not require --local.

Tests

  • My PR does not need testing for this extremely good reason:
    It is not entirely clear how to test this flag, or if it makes sense. I'm happy to add one if you have ideas!

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":

    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"
  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@Fokko
Copy link
Contributor

Fokko commented Jan 22, 2018

Hi @yati-sagade, this PR makes sense to me. It is related to the code @bolkedebruin wrote. Wouldn't be airflow test more appropriate to test the dag instead of airflow run, what's your opinion on that?

@yati-sagade
Copy link
Contributor Author

Hey @Fokko thanks for reviewing. Indeed airflow test is used more than run to actually test tasks, but for example, the scheduler enqueues a airflow run --local ... command to the executors, and these are then run verbatim by the executors. In my foray into the scheduler and executor guts to fix AIRFLOW-2001, I felt the need to trigger the whole flow (as opposed to locally running a ti with airflow test), especially since a promising fix for that issue looks like treating different nonzero exit codes from the airflow run --local ... command executed on workers differently (currently any nonzero code means error). What do you think?

@Fokko
Copy link
Contributor

Fokko commented Jan 26, 2018

Hi @yati-sagade, yes that makes sense. This can also be interesting for testing an Airflow job in CI. Can you fix the flake8 tests?

@yati-sagade
Copy link
Contributor Author

yati-sagade commented Jan 27, 2018

@Fokko Thanks, fixed mine, what is the policy on all other flake8 violations in the file (as opposed to just my patch)? Do I need a separate JIRA ticket to fix those as well, or just a PR would do?

We capture the standard output and error streams so that they're handled
by the configured logger. However, sometimes, when developing dags or
Airflow code itself, it is useful to put pdb breakpoints in code
triggered using an `airflow run`. Such a flow would of course require
not redirecting the output and error streams to the logger.

This patch enables that by adding a flag to the `airflow run`
subcommand. Note that this does not require `--local`.
@codecov-io
Copy link

Codecov Report

Merging #2957 into master will increase coverage by 0.09%.
The diff coverage is 56.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2957      +/-   ##
==========================================
+ Coverage   73.13%   73.23%   +0.09%     
==========================================
  Files         175      175              
  Lines       12325    12329       +4     
==========================================
+ Hits         9014     9029      +15     
+ Misses       3311     3300      -11
Impacted Files Coverage Δ
airflow/bin/cli.py 54.8% <56.66%> (+0.12%) ⬆️
airflow/models.py 87.04% <0%> (+0.04%) ⬆️
airflow/jobs.py 79.94% <0%> (+0.44%) ⬆️
airflow/utils/helpers.py 54.02% <0%> (+2.87%) ⬆️
airflow/task/task_runner/bash_task_runner.py 100% <0%> (+6.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1d5551...7176ae8. Read the comment docs.

@asfgit asfgit closed this in efd8338 Jan 28, 2018
@Fokko
Copy link
Contributor

Fokko commented Jan 28, 2018

Thanks @yati-sagade

aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
We capture the standard output and error streams
so that they're handled
by the configured logger. However, sometimes, when
developing dags or
Airflow code itself, it is useful to put pdb
breakpoints in code
triggered using an `airflow run`. Such a flow
would of course require
not redirecting the output and error streams to
the logger.

This patch enables that by adding a flag to the
`airflow run`
subcommand. Note that this does not require
`--local`.

Closes apache#2957 from yati-sagade/ysagade/airflow-2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants