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

Error for unhashable types used as Keys in the engine is unfortunate #7499

Closed
stuhood opened this issue Apr 4, 2019 · 0 comments

Comments

Projects
None yet
1 participant
@stuhood
Copy link
Member

commented Apr 4, 2019

Getting a Key for a Value in the engine requires hashing it, and when that fails, the error message is unfortunate (because we do not currently treat Interns::put or externs::identify as fallible operations):

From cffi callback <bound method _FFISpecification.extern_identify of <pants.engine.native._FFISpecification object at 0x10a100510>>:
Traceback (most recent call last):
  File "/Users/bescobar/workspace/otherpants/src/python/pants/engine/native.py", line 283, in extern_identify
    hash_ = hash(obj)
  File "/Users/bescobar/workspace/otherpants/src/python/pants/util/objects.py", line 107, in __hash__
    return super(DataType, self).__hash__()
TypeError: unhashable type: 'list'
thread 'tokio-runtime-worker-2' panicked at 'WithDeps(Inner(InnerEntry { params: {TypeId(140728165282080)}, rule: Task(Task { product: TypeConstraint(Key { id: 74, type_id: TypeId(140728165231312) }), clause: [Select { product: =Console }, Select { product: =TransitiveHydratedTargets }], gets: [Get { product: TypeConstraint(Key { id: 76, type_id: TypeId(140728165231312) }), subject: TypeId(140728202990560) }], func: Function(Key { id: 75, type_id: TypeId(4418260136) }), cacheable: false }) })) did not declare a dependency on JustGet(Get { product: TypeConstraint(Key { id: 76, type_id: TypeId(140728165231312) }), subject: TypeId(0) })', src/nodes.rs:839:13

While not all extern methods should be fallible (although they can effectively all fail due to python exceptions), this one is probably a good candidate. Most of the objects that pass through the engine are Values, and relatively few need to be converted into Keys: essentially, only those used as Params (eg. as arguments to Get(..)). But introducing new calls to Get(..) is a much more common operation, and rendering a good error message in this case would be valuable.

To make this cheaper, and provide a better illusion of synchronous behaviour when you call Get with a bad argument, one approach would be to change the signature of extern_generator_send to directly return pre-identified handles (ie, including both the Handle, and an Ident as from extern_identify) when yielding a Get/GetMulti. This would make the PyGeneratorResponse struct more complicated though, because neither the Break nor Throw cases should identify their arguments.

@stuhood stuhood added the engine label Apr 4, 2019

@stuhood stuhood self-assigned this Apr 4, 2019

stuhood added a commit that referenced this issue Apr 10, 2019

Improve Get errors for unhashable or mismatched types (#7502)
### Problem

The error for unhashable types used as `Get` params is unfortunate, because the call to `identify` fails and returns a null pointer to rust, which triggers an error... somewhere else. See #7499.

### Solution

As suggested in #7499, have `extern_generator_send` directly `identify` `Get` params, meaning that failures to hash take advantage of the fact that `extern_generator_send` is already fallible. This avoids a significant number of calls back to python to identify `Values` (an optimization discussed in #7318 and #7114).

### Result

Improved usability. Fixes #7499.
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.