Skip to content

Comments

[AIRFLOW-2099] Handle getsource() calls gracefully#3571

Closed
night0wl wants to merge 1 commit intoapache:masterfrom
night0wl:AIRFLOW-2099_task_view_type_check
Closed

[AIRFLOW-2099] Handle getsource() calls gracefully#3571
night0wl wants to merge 1 commit intoapache:masterfrom
night0wl:AIRFLOW-2099_task_view_type_check

Conversation

@night0wl
Copy link
Contributor

@night0wl night0wl commented Jul 3, 2018

There are several scenarios where Task Instance view tries to render
Python callables where 'x' is not the correct artefact to target.

This commit adds a helper fuction to test for known scenarios, and
derives the source from the correct artifact or as a default returns
'No source available for '. This means that even in unknown or
unfixable edge cases, the Task Instance view still renders instead of
displaying an exception.

Make sure you have checked all steps below.

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:
  1. Adds a wrapper function for getting Python source code.
  2. It tests for/catches known anomalies and implements a work around, finally returning 'source code not available for ' if no option works.

Tests

  • My PR adds the following unit tests
  1. test_dag_view_task_with_python_operator_using_partial
  2. test_dag_view_task_with_python_operator_using_instance

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

@night0wl night0wl force-pushed the AIRFLOW-2099_task_view_type_check branch 2 times, most recently from 6e3d71f to 09d0a42 Compare July 3, 2018 11:31
@codecov-io
Copy link

codecov-io commented Jul 3, 2018

Codecov Report

Merging #3571 into master will decrease coverage by 58.94%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3571       +/-   ##
===========================================
- Coverage   76.83%   17.89%   -58.95%     
===========================================
  Files         204      204               
  Lines       15492    15510       +18     
===========================================
- Hits        11904     2775     -9129     
- Misses       3588    12735     +9147
Impacted Files Coverage Δ
airflow/www/views.py 0% <ø> (-68.95%) ⬇️
airflow/www/utils.py 0% <0%> (-87.06%) ⬇️
airflow/operators/email_operator.py 0% <0%> (-100%) ⬇️
airflow/www/forms.py 0% <0%> (-100%) ⬇️
...le_dags/example_passing_params_via_test_command.py 0% <0%> (-100%) ⬇️
...w/example_dags/example_latest_only_with_trigger.py 0% <0%> (-100%) ⬇️
airflow/example_dags/example_docker_operator.py 0% <0%> (-100%) ⬇️
airflow/www_rbac/blueprints.py 0% <0%> (-100%) ⬇️
airflow/www/validators.py 0% <0%> (-100%) ⬇️
airflow/operators/sensors.py 0% <0%> (-100%) ⬇️
... and 160 more

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 9ebb04a...331bb51. Read the comment docs.

@ashb
Copy link
Member

ashb commented Jul 6, 2018

Could you add some tests that directly call wwwutils.get_python_source with one case for each type?

@night0wl
Copy link
Contributor Author

night0wl commented Jul 10, 2018

@ashb no problem, they should go in tests/www/test_utils.py in the UtilsTest TestCase I assume.

"One case for each type", this would be 4; one for callable class, one for method, one for partial function, and one for None

@ashb
Copy link
Member

ashb commented Jul 10, 2018

Yes to both questions.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Remove the print and squash to one commit in this case please :)

Copy link
Member

Choose a reason for hiding this comment

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

Probably shouldn't leave this in here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops!

@night0wl night0wl force-pushed the AIRFLOW-2099_task_view_type_check branch 2 times, most recently from 992e7f7 to e18d1db Compare July 10, 2018 20:18
There are several scenarios where Task Instance view tries to render
Python callables where 'x' is not the correct artefact to target.

This commit adds a helper fuction to test for known scenarios, and
derives the source from the correc artefact or as a default returns
'No source available for <type>'. This means that even in unknown or
unfixable edge cases, the Task Instance view still renders instead of
displaying an exception.

AIRFLOW-2099 Add get_python_source unit tests

AIRFLOW-2099 Add AttributeError to __call__ except
@night0wl night0wl force-pushed the AIRFLOW-2099_task_view_type_check branch from e18d1db to 331bb51 Compare July 11, 2018 07:11
@night0wl
Copy link
Contributor Author

@ashb any idea what's up with codecov? I rebased again onto master to make sure everything was up-to-date.

@ashb
Copy link
Member

ashb commented Jul 11, 2018

Codecov has a bit of a "wobble" and goes up and down in small numbers - I've never dug into why but it doesn't worry me.

Oh, that it hasn't run. 🤷 - again not concerning me much.

@ashb ashb closed this in a4b9aa3 Aug 2, 2018
lxneng pushed a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018
There are several scenarios where Task Instance view tries to render
Python callables where 'x' is not the correct artefact to target.

This commit adds a helper fuction to test for known scenarios, and
derives the source from the correc artefact or as a default returns 'No
source available for <type>'. This means that even in unknown or
unfixable edge cases, the Task Instance view still renders instead of
displaying an exception.

Closes apache#3571 from night0wl/AIRFLOW-2099_task_view_type_check
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
There are several scenarios where Task Instance view tries to render
Python callables where 'x' is not the correct artefact to target.

This commit adds a helper fuction to test for known scenarios, and
derives the source from the correc artefact or as a default returns 'No
source available for <type>'. This means that even in unknown or
unfixable edge cases, the Task Instance view still renders instead of
displaying an exception.

Closes apache#3571 from night0wl/AIRFLOW-2099_task_view_type_check
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