[engine] Use Value in invoke_runnable as the function instead of Function #4258

Merged
merged 3 commits into from Feb 14, 2017

Conversation

Projects
None yet
2 participants
@baroquebobcat
Contributor

baroquebobcat commented Feb 14, 2017

Problem

Sometimes we want to be able to invoke Python functions from Rust that are not pickleable. Right now, we use the Function struct. It requires a Key to have been constructed for the function in order to use it. Building keys requires pickling, so unpickleable functions cannot be called from Rust.

Solution

Use Value instead of Function for runnable calls. Value doesn't have to require a pickle, so unpickleable functions can be represented with it.

The one funny bit here is Key::new_with_anon_type_id. val_for doesn't check the type id of the passed key, so this works. It could be cleaner though.

@stuhood

Thanks.

src/rust/engine/src/core.rs
@@ -88,6 +88,10 @@ impl hash::Hash for Key {
}
impl Key {
+ pub fn new_with_anon_type_id(id: Id) -> Key {
+ Key { id: id, type_id: TypeId(0) }

This comment has been minimized.

@stuhood

stuhood Feb 14, 2017

Member

Unless this id is reserved on the python side, this won't really be anonymous...

@stuhood

stuhood Feb 14, 2017

Member

Unless this id is reserved on the python side, this won't really be anonymous...

This comment has been minimized.

@@ -87,6 +87,10 @@ impl Externs {
(self.val_for)(self.context, key)
}
+ pub fn val_for_id(&self, id: Id) -> Value {
+ self.val_for(&Key::new_with_anon_type_id(id))

This comment has been minimized.

@stuhood

stuhood Feb 14, 2017

Member

Would be awesome to remove the type_id from Value if possible (and leaving it on Key)... looks like of the 2 or 3 places it is used, all of the cases would likely be OK with computing to a Key first.

@stuhood

stuhood Feb 14, 2017

Member

Would be awesome to remove the type_id from Value if possible (and leaving it on Key)... looks like of the 2 or 3 places it is used, all of the cases would likely be OK with computing to a Key first.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 14, 2017

Contributor

Hm. By that do you mean it'd be worth doing as part of this review, or can it wait as another small change?

@baroquebobcat

baroquebobcat Feb 14, 2017

Contributor

Hm. By that do you mean it'd be worth doing as part of this review, or can it wait as another small change?

@baroquebobcat baroquebobcat merged commit 9ae05fc into pantsbuild:master Feb 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] Use Value in invoke_runnable as the function instead of Func…
…tion (#4258)

### Problem

Sometimes we want to be able to invoke Python functions from Rust that are not pickleable. Right now, we use the `Function` struct. It requires a `Key` to have been constructed for the function in order to use it. Building keys requires pickling, so unpickleable functions cannot be called from Rust.

### Solution

Use Value instead of Function for runnable calls. Value doesn't have to require a pickle, so unpickleable functions can be represented with it.

The one funny bit here is `Key::new_with_anon_type_id`. `val_for` doesn't check the type id of the passed key, so this works. It could be cleaner though.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment