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

add stack traces to unhashable TypeErrors in the engine #7532

Conversation

Projects
None yet
5 participants
@cosmicexplorer
Copy link
Contributor

commented Apr 11, 2019

Problem

  • When we provide an unhashable type such as list to a datatype field of an object that goes through the v2 engine, the engine will tell you there is a TypeError, but won't tell you which object it's from.
  • The unhashable type error happens late (long after the datatype is constructed) and without an appropriate stacktrace. It should be easy to declare hashable collection fields (in this case, just tuples) and have them fail early (at construction).

Solution

  • Add the name of the type of the object passed to extern_identify() to the TypeError raised if one of its fields are unhashable.
  • Make a HashableTypedCollection type constructor.
  • Capture exceptions raised in CFFI extern methods using the onerror handler and re-raise those with their tracebacks so the TypeErrors are actually catchable.

Result

Unhashable type errors for datatypes going through the engine happen less, and errors happen earlier and with more context!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:better-unhashable-type-engine-error branch from 4ef01bb to f014112 Apr 11, 2019

@cosmicexplorer cosmicexplorer requested review from blorente and ity Apr 11, 2019

@illicitonion
Copy link
Contributor

left a comment

Nice improvement! Thanks!

Show resolved Hide resolved src/python/pants/engine/native.py Outdated
Show resolved Hide resolved src/python/pants/util/objects.py Outdated
self.scheduler.product_request(C, [Params(TypeCheckFailWrapper(A()))])

with assert_execution_error(self, dedent("""\
TypeError: For object with type 'CollectionType', error was: unhashable type: 'list'""")):

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 11, 2019

Contributor

I don't suppose it's easy/possible to add the field name here too, is it?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 11, 2019

Author Contributor

We probably could, by modifying the __hash__ of datatype. I will look into this since that is a clear usability improvement.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 11, 2019

Author Contributor

Done -- was able to remove the change in native.py as a result!

@ity

ity approved these changes Apr 11, 2019

Copy link
Contributor

left a comment

looks good, sans Daniel's comments

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:better-unhashable-type-engine-error branch from b953b2e to a551a04 Apr 11, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

The added test fails because the TypeError in the datatype __hash__ happens to be uncatchable for some reason (it just gets logged to the console) -- will figure out why.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:better-unhashable-type-engine-error branch 3 times, most recently from bf41236 to 476b8ad Apr 14, 2019

@cosmicexplorer cosmicexplorer changed the title Better unhashable type engine error add stack traces to unhashable TypeErrors in the engine Apr 14, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Had to go on a bit of a journey to find the CFFI docs and realize it just throws away the exception after logging to stderr, but found a way to get around that with a custom onerror handler, so this should finally work and pass tests.

@illicitonion
Copy link
Contributor

left a comment

Thanks! Couple of trivial comments, couple of things worth thinking about a little before merging

Show resolved Hide resolved src/python/pants/engine/scheduler.py Outdated
request = None
try:
request = self.execution_request([product], subjects)
except Exception:

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 15, 2019

Contributor

Swallowing this exception in the case that the engine had errors seems bad. Can we add it to the list of error tracebacks?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 15, 2019

Author Contributor

Yes! The exception itself is however usually unrelated, e.g. something like:

No @rules were found to satisfy X for the input Select(Any+Y)

since CFFI extern methods return None when the error is captured. I think it's safer to add these things and remove them later though, so I will add that to the end of the list of tracebacks, with some prefix like:

The engine execution request returned this error, which is probably because of the errors in the CFFI extern methods listed above.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 16, 2019

Author Contributor

Done!

Show resolved Hide resolved src/python/pants/engine/scheduler.py Outdated
1 Exception raised in CFFI extern methods:
Traceback (most recent call last):
File LOCATION-INFO, in __hash__
return super(DataType, self).__hash__()

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 15, 2019

Contributor

It seems weird that this appears twice; once in the subclass constructor and once in the superclass; that seems like a pretty noisy thing to be asserting on... Is it possible to get rid of one of them?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 15, 2019

Author Contributor

Sure, I guess I just wanted to ensure the full traceback is printed, since the current assert_execution_error() contextmanager happens to make matching against the entire string easier than matching multiple substrings of it. I think this could and should be converted to just checking the one traceback entry (which can then be the same for both py2 and py3), and then separately that the exception message itself is as we expect.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 16, 2019

Author Contributor

I've modified the testing to assert something much less brittle, and in the process extended the assertions to cover the other ways that method can fail!

@blorente
Copy link
Contributor

left a comment

This is awesome, thanks!
I'm probably not qualified to check the "dots on the i's" on this one, but I think the design is solid and there should be no big perf regression afaict.

@@ -224,15 +240,55 @@ def test_get_type_match_failure(self):

def test_unhashable_failure(self):
"""Test that unhashable Get(...) params result in a structured error."""
expected_msg = """TypeError: unhashable type: 'list'"""
with assert_execution_error(self, expected_msg):
with assert_execution_error(self, "TypeError: unhashable type: 'list'"):

This comment has been minimized.

Copy link
@blorente

blorente Apr 15, 2019

Contributor

nit. Can we delete this test, or split it into two to separate the two failure conditions?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 15, 2019

Author Contributor

Split the test method into two test methods, you mean? I wanted to have that assertion here to check that some appropriate error message is produced for non-datatype objects as well. It could be split into a separate method if necessary, although I see it as being closely related to the datatype hash() assertion, unless I'm missing something?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 16, 2019

Author Contributor

I've deleted that assertion, but also added a couple more. They are all in the same method as they share some checking logic.

@illicitonion
Copy link
Contributor

left a comment

Great! Thanks!

raise

# We still want to raise whenever there are any exceptions in any CFFI extern methods, even if
# that didn't lead to an exception in generating the execution request for some reason, so we
# check the extern exceptions list again.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 16, 2019

Contributor

We could get rid of a bunch of the repeated conditional checks, and make this read a bit better, by just structuring this except block as:

except:
  internal_errors = self._scheduler._native.cffi_extern_method_runtime_exceptions()
  if not internal_errors:
    raise
  error_tracebacks = [...]
  exc_type, exc_value, tv = sys.exc_info()
  ...

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 16, 2019

Author Contributor

So there's no guarantee that the .execution_request() method will raise an error just because the CFFI externs do -- that is currently the case, because our new onerror handler will make them return None when an exception is raised, and the current python and rust code that runs in the .execution_request() method will end up raising an error if those extern methods return None, but that's not behavior I want to depend on, hence checking outside of the except as well.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 16, 2019

Author Contributor

And I can't see how to merge that logic with e.g. generating the error_tracebacks within the except.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 17, 2019

Contributor

Ohhhhh I totally didn't see the fact that this is less indented than the block above! Apologies!

cosmicexplorer added some commits Apr 11, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:better-unhashable-type-engine-error branch from 8783013 to c95979d Apr 16, 2019

@cosmicexplorer cosmicexplorer merged commit fac27b8 into pantsbuild:master May 12, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.