You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/31 15:11:48 UTC
[GitHub] [airflow] jedcunningham opened a new pull request #10663: Optional import error tracebacks in web ui
jedcunningham opened a new pull request #10663:
URL: https://github.com/apache/airflow/pull/10663
This PR allows for partial import error tracebacks to be exposed on the UI, if enabled. This extra context can be very help for users without access to the parsing logs to determine why their DAGs are failing to import properly.
Disabled:
Enabled:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #10663: Optional import error tracebacks in web ui
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#issuecomment-683841036
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itβs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://apache-airflow-slack.herokuapp.com/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#issuecomment-690846446
@ashb, changed the default to true, refactored so we only hit conf once, and gave the tests a little more love. The only remaining thing I can think of is `version_added`, not sure if you all are planning to backport this to 1.x.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r485718422
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('webserver', 'dagbag_import_error_tracebacks'):
Review comment:
@ashb, does π mean you think we should do attrs?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek merged pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #10663:
URL: https://github.com/apache/airflow/pull/10663
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10663: Optional import error tracebacks in web ui
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r485068434
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('webserver', 'dagbag_import_error_tracebacks'):
Review comment:
Perhaps just under `core`? (This feels slightly wrong here as this code isn't actually run in the webserver on 2.0, but under the scheduler)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r486242691
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('core', 'dagbag_import_error_tracebacks'):
Review comment:
Only if
https://github.com/apache/airflow/blob/ce66bc944d246aa3b51cce6e2fc13cd25da08d6e/airflow/configuration.py#L338
So if users will not update their config they will face an unknown error
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#issuecomment-694709824
> @ashb, would you like me to rebase to see if the tests go green?
@jedcunningham yes, please rebase
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#issuecomment-694330469
@ashb, would you like me to rebase to see if the tests go green?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r486521676
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('core', 'dagbag_import_error_tracebacks'):
Review comment:
Yup.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r485725736
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('core', 'dagbag_import_error_tracebacks'):
Review comment:
https://github.com/apache/airflow/blob/master/airflow/configuration.py#L334
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#issuecomment-690846446
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10663: Optional import error tracebacks in web ui
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r485065853
##########
File path: airflow/www/static/css/main.css
##########
@@ -295,6 +295,10 @@ label[for="timezone-other"],
margin-bottom: 15px;
}
+.dag-import-error {
+ white-space: pre;
+}
+
Review comment:
You found the "right" place (or as right a place as exists.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r486531848
##########
File path: airflow/config_templates/config.yml
##########
@@ -229,6 +229,21 @@
type: string
example: ~
default: "30"
+ - name: dagbag_import_error_tracebacks
+ description: |
+ Should a traceback be shown in the UI for dagbag import errors,
+ instead of just the exception message
+ version_added: 2.0.0
+ type: boolean
+ example: ~
+ default: "False"
Review comment:
Works for me. Running into some test failures by changing the default, but I'll get them worked out.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #10663: Optional import error tracebacks in web ui
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r485198197
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('webserver', 'dagbag_import_error_tracebacks'):
Review comment:
Yeah, looking with fresh eyes, `core` is a much better fit.
Think we should also set attributes like we do for `DAGBAG_IMPORT_TIMEOUT` so we only hit the config system once?
e.g. https://github.com/apache/airflow/blob/master/airflow/models/dagbag.py#L80
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10663: Optional import error tracebacks in web ui
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r485464864
##########
File path: airflow/config_templates/config.yml
##########
@@ -229,6 +229,21 @@
type: string
example: ~
default: "30"
+ - name: dagbag_import_error_tracebacks
+ description: |
+ Should a traceback be shown in the UI for dagbag import errors,
+ instead of just the exception message
+ version_added: 2.0.0
+ type: boolean
+ example: ~
+ default: "False"
Review comment:
Default of True is probably okay -- I think this is a long term complaint and I can see very few reasons to have it off (by default)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#issuecomment-702320416
@turbaszek @ashb I rebased to rekick the tests... not sure why that one test got killed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r486513326
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('core', 'dagbag_import_error_tracebacks'):
Review comment:
`self.airflow_defaults` is a parsed version of `default_airflow.cfg`, so I don't think we need to double up with a fallback here. They won't run into errors if they don't add stuff to their `airflow.cfg`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #10663: Optional import error tracebacks in web ui
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r480198305
##########
File path: airflow/www/static/css/main.css
##########
@@ -295,6 +295,10 @@ label[for="timezone-other"],
margin-bottom: 15px;
}
+.dag-import-error {
+ white-space: pre;
+}
+
Review comment:
I wasn't exactly where in here this should live, so this placement is arbitrary π€·ββοΈ
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#issuecomment-695021112
Added `traceback` and `tracebacks` to the spelling wordlist, not sure why "Wait for PROD images" got stuck...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#issuecomment-690846446
@ashb, changed the default to true, refactored so we only hit conf once, and gave the tests a little more love. The only remaining thing I can think of is `version_added`, not sure if you all are planning to backport this to 1.x.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #10663: Optional import error tracebacks in web ui
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r480197796
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('webserver', 'dagbag_import_error_tracebacks'):
Review comment:
I put the config under webserver as I thought that made the most sense for the end user. Happy to move them elsewhere if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10663: Show a few lines of tracebacks for DAG import errors in web UI
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10663:
URL: https://github.com/apache/airflow/pull/10663#discussion_r485720541
##########
File path: airflow/models/dagbag.py
##########
@@ -269,7 +270,12 @@ def _load_modules_from_file(self, filepath, safe_mode):
return [new_module]
except Exception as e: # pylint: disable=broad-except
self.log.exception("Failed to import: %s", filepath)
- self.import_errors[filepath] = str(e)
+ if conf.getboolean('core', 'dagbag_import_error_tracebacks'):
Review comment:
Should we add `fallback` just in case someone don't have this option in config?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org