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

Better Python Errors #9398

Merged
merged 23 commits into from
Sep 8, 2020
Merged

Conversation

johnc1231
Copy link
Contributor

@johnc1231 johnc1231 commented Sep 2, 2020

This PR is based on discussion here: https://dev.hail.is/t/better-python-error-messages-idea/201/9 The intention is to create a system to give better error messages from python in a generic way. Tim's work in #7792 does a good job introducing behavior like this this specifically for ArrayRef nodes, but I want to add three things on top of that:

  1. I don't want to have to do as much custom per IR node work
  2. I don't want to send the entire python stack trace over py4j to scala for every node
  3. I don't want the user to see a Java stack trace in this scenarios.

This first PR is a proof of concept that adds this behavior for the Die node, which will catch any errors generated by uses of CaseBuilder.or_error. Follow up PRs should change ArrayRef to work this way, as well as catch things like looking up a key in a dictionary but not finding it. In an ideal future, we'd bolt on some extra mechanism to give types to these errors, and we could throw a proper IndexError in the ArrayRef case or KeyError in the dictionary case.

It feels a little bit messy right now, open to suggestions. I don't love using -1 as the "no error" situation, but I thought it was probably easier than dealing with optionals between python and scala.

To give an example of what it looks like, the error message for this script:

import hail as hl

ht = hl.utils.range_table(10)
ht = ht.annotate(foo = hl.nd.array([[1], [2], [3]]))
ht = ht.annotate(bar = ht.foo[0:4, 12])
ht.collect()

is

Traceback (most recent call last):
  File "better_error_test.py", line 6, in <module>
    ht.collect()
  File "<decorator-gen-1103>", line 2, in collect
  File "/Users/johnc/Code/hail/hail/python/hail/typecheck/check.py", line 614, in wrapper
    return __original_func(*args_, **kwargs_)
  File "/Users/johnc/Code/hail/hail/python/hail/table.py", line 1903, in collect
    return Env.backend().execute(e._ir)
  File "/Users/johnc/Code/hail/hail/python/hail/backend/spark_backend.py", line 325, in execute
    raise HailUserError(message_and_trace) from None
hail.utils.java.HailUserError: Error summary: HailException: Index 12 is out of bounds for axis 1 with size 1
------------
Hail stack trace:
  File "better_error_test.py", line 5, in <module>
    ht = ht.annotate(bar = ht.foo[0:4, 12])

  File "<decorator-gen-707>", line 2, in __getitem__

  File "/Users/johnc/Code/hail/hail/python/hail/expr/expressions/typed_expressions.py", line 3709, in __getitem__
    hl.str("Index ") + hl.str(s) + hl.str(f" is out of bounds for axis {i} with size ") + hl.str(dlen)

  File "<decorator-gen-1299>", line 2, in or_error

  File "/Users/johnc/Code/hail/hail/python/hail/expr/builders.py", line 311, in or_error
    die_ir.save_error_info()

(Note that ndarray bounds checking internally uses a case builder, which is why this example works).

@johnc1231
Copy link
Contributor Author

One other question: Is it a breaking change to change the type of error we throw?

Copy link
Collaborator

@patrick-schultz patrick-schultz left a comment

Choose a reason for hiding this comment

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

This is great. Overall design looks good, just a few minor things.

Comment on lines 219 to 224
while i < len(stack):
candidate = stack[i]
i += 1
if any(phrase in candidate for phrase in forbidden_phrases):
continue
filt_stack.append(candidate)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be simplified to

filt_stack = [
    candidate for candidate in stack[i:]
              if not any(phrase in candidate for phrase in forbidden_phrases)
]

@@ -1685,15 +1685,15 @@ class Emit[C](
val ev = mb.getEmitParam(2 + i)
assert(ev.pt == expectedPType)
ev
case Die(m, typ) =>
case @Die(m, typ, errorId) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extraneous @

@@ -1508,7 +1508,7 @@ case class TableMapRows(child: TableIR, newRow: IR) extends TableIR {
FastIndexedSeq(classInfo[Region], LongInfo, LongInfo), LongInfo,
Coalesce(FastIndexedSeq(
extracted.postAggIR,
Die("Internal error: TableMapRows: row expression missing", extracted.postAggIR.typ))))
Die("Internal error: TableMapRows: row expression missing", extracted.postAggIR.typ, -1))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You added the constructor that sets the errorId to -1 implicitly; did you not want to use that here (and several other places)? That would keep the -1 magic number hard coded in fewer places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I originally added the numeric argument everywhere manually because I didn't want to miss anywhere where I had to copy the argument along. I can go back now and remove the extras.

@johnc1231
Copy link
Contributor Author

Thanks for the review, all addressed now.

@@ -1,5 +1,6 @@
package is.hail.expr.ir

import com.google.api.gax.rpc.InvalidArgumentException
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that was a mistake where I typed "Invalid" instead of "Illegal", must have autoimported.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I do that all the time!

@danking danking merged commit 0eb2752 into hail-is:main Sep 8, 2020
pwc2 pushed a commit to pwc2/hail that referenced this pull request Sep 21, 2020
* Added error_id to Die in python

* WIP on better errors

* WIP, I'm now saving an error id in Java and sending it back to Python

* Added an ir search to find the ir node where an exception originates. Now need to pass an error id for Die to make an example

* Made the connection, just have to cleanup a bit now

* Seems to be working

* Actually working

* Remove pdb

* Let's call it hail stack trace instead

* IRSuite test fixes

* Updated python tests for case builders to verify this is working

* Removed extra @ sign

* Don't explicitly pass -1 in TableIR

* Change filtering approach, add decorator-gen

* Delete InvalidArgumentException

* Style fixes

* Fixed bad HailException constructor

* Added HailUserError to things that are exported, catching the right error type in bit shift

* Fixed slice and eval tests

* biallelic and uniroot throw user errors now too

* Fix uniroot and locus windows

* locus_windows actually passing now

* Copying Die nodes need to preserve the stack trace and error id
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.

None yet

4 participants