Skip to content

Conversation

@dossett
Copy link
Contributor

@dossett dossett commented Jun 7, 2019

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title.

Description

  • Here are some details about my PR, including screenshots of any UI changes: provide extra error handling information from failed dag parsing

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

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 (not including Jira issue reference)
    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"

Code Quality

  • Passes flake8

@dossett
Copy link
Contributor Author

dossett commented Jun 9, 2019

One of the test suites passed, some others failed, I can't tell if the failures are connected to this change or not.

dossett and others added 3 commits June 10, 2019 09:52
Co-Authored-By: Chao-Han Tsai <milton0825@gmail.com>
Co-Authored-By: Chao-Han Tsai <milton0825@gmail.com>
@dossett
Copy link
Contributor Author

dossett commented Jun 12, 2019

Failing unit tests fixed. Searching for [fail] in the travis output was the key.

Thanks for the suggestion @milton0825

@ashb
Copy link
Member

ashb commented Jun 12, 2019

Do you fancy a bit of a deeper change here? Rather than just making import errors a list of strings, seei if we could make it a list of dicts of {'error': str(e), 'traceback': traceback.format_exc() } and then we can improve the rendering of this in the UI. This may turn out to be a much bigger change though, but would I think produce a nicer result.

@dossett dossett closed this Jun 13, 2019
@dossett dossett reopened this Jun 13, 2019
@dossett
Copy link
Contributor Author

dossett commented Jun 13, 2019

@ashb That is an intriguing idea, but I don't have the bandwidth of a broader fix right now. This is one of a few Airflow PRs I'm working as a result our 1.10.3 upgrade last week.

@dossett
Copy link
Contributor Author

dossett commented Jun 17, 2019

@ashb Are you ok with this more limited change?

@ashb
Copy link
Member

ashb commented Jun 18, 2019

@dossett Could you provide a screenshot of what this error looks like currently?

I have a feeling we may need to add whitespace: pre-wrap to the CSS somewhere otherwise the stack trace may be unreadable.

@dossett
Copy link
Contributor Author

dossett commented Jun 18, 2019

@ashb The way we use Airflow I don't actually see this in the UI. We have a service to submit the DAGs which calls into Airflow's code to parse the dag first (I don't recall the exact mechanism), so I only see this in an error log.

@dossett
Copy link
Contributor Author

dossett commented Jun 18, 2019

@ashb I stand corrected, I saw it the next time I opened Airflow, lol.

image

@ashb
Copy link
Member

ashb commented Jun 18, 2019

Hmmmmm - that is rather large and long. Adding the CSS rule I suggested would make it take up about 3 screens. Is that error any use as it stands to you?

@dossett
Copy link
Contributor Author

dossett commented Jun 19, 2019

@ashb No, this particular error isn't that useful, but other errors have been especially very generic python error messages. It does seem clunky in the UI like this, I admit, but finding it in a log file is super valuable.

@pgagnon
Copy link
Contributor

pgagnon commented Jun 21, 2019

@ashb @dossett I think this is a useful change but the current implementation perhaps adds a bit too much clutter. I could see situations where you have five of these errors taking an inconvenient amount of space. Perhaps it would be better if the stack trace details were hidden behind some kind of expandable details panel?

@ashb
Copy link
Member

ashb commented Jun 21, 2019

@pgagnon Yeah, that's what I was thinking with #5386 (comment) -- I just worry that this is going to take up too much space on the screen.

@pgagnon
Copy link
Contributor

pgagnon commented Jun 21, 2019

Also nit: The PR title is missing brackets to encapsulate the JIRA id. I'm not sure if this is an actual requirement but it would be more consistent.

@ashb
Copy link
Member

ashb commented Jul 15, 2019

Going to close this PR for now as the messages it prints will likely be too long and will make a mess of the UI. Sorry. It'll be a nice feature though

@ashb ashb closed this Jul 15, 2019
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.

4 participants