-
Notifications
You must be signed in to change notification settings - Fork 428
Add SavedModelBundle API #90
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
Conversation
I originally left out proto support because TensorFlow uses proto3 and no Rust libraries at the time supported proto3. We'll have to decide at some point whether to leave serialization/deserialization to the user, or to pick from https://github.com/stepancheg/rust-protobuf or something else. If we choose a proto library, we should be able to write the API in a way that hides the choice of library from the user, but it's still a commitment. @jhseu, do you have any thoughts on TensorFlow proto concerns? I see that the Go and Java bindings expose the config proto as a raw byte array to the user. I'd prefer any public APIs to use full-fledged structs rather than tuples or tuple structs. They have slightly more boilerplate, but they're also more extensible and explicit. Within the implementation, I think tuples and tuple structs are fine. As for the method name, to keep more consistent with the Go API and the Java API, I think it would make sense to move it to |
That is now done in 43f6bf9. |
Yeah, leaving serialization/deserialization up to the user seems like the most reasonable option given the fragmented protobuf situation in Rust. If there were a hard dependency, prost seems like the most reasonable protobuf crate right now, but it's too new to depend on. |
I can't speak for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test?
src/session.rs
Outdated
let mut status = Status::new(); | ||
|
||
let export_dir_cstr = | ||
try!(export_dir.as_ref() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For new code, ? is preferred over try!.
src/session.rs
Outdated
pub struct SavedModelBundle { | ||
/// The loaded session. | ||
pub session: Session, | ||
/// A meta graph defition as raw protocol buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "definition"
Is it ok for me to add a "test-resources" folder with a known saved model for this? |
Sure, go ahead. |
- Give explicit name to an operation in regression_savedmodel script - Add test_resources with a saved model - Add test for saved model bundle - Fix typo - Wipe try! macros
The requested changes were made. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* tensorflow#89 Remove fold/sheet limits so a full dataset can be output, albeit still limited by DisplayMinWidth/column concatenation at header width * Update Readme with arguments
This is more of a request for feedback rather than something ready to merge now. As this concern was exposed in #68, I built an API that gathers the loaded session and meta-graph information into a bundle. When combined with the latest
protobuf
crate and protoc-generated sources, it is then possible to retrieve signature definitions from the meta-graph with something like this:Since this crate is still independent from
protobuf
, this extra constructor method currently provides a vector of raw bytes.Feedback would be certainly welcome. In particular, is the method's name ok? Should
SavedModelBundle
be just a tuple? Or maybe a tuple struct? This API might also depend on whether this crate will have its own protobuf serialization/deserialization code in the future. It seems that we cannot obtain certain features already available in the Python API without this.