Closed chrhansen closed 8 years ago
@chrhansen The main reason I am not changing the protobuf gem is relatively lower speed and that's it. When I run the tests on ruby-protocol buffers, its a bit slower. And as for your other questions about "would it be possible to update only those changes (gemfile and the stuff in lib/tensorflow/core) in a separate PR today, while you polish this image-recognition PR?"
Then yes, This is possible and I can do it with just a few minor changes to any files other than lib/tensorflow/core.
@Arafatk You're the protobuf-gem expert, so you decide that :)
ruby-protocol-buffers
-gem instead of google-protoc
? ruby-protocol-buffers
do you think?If google-protoc
-gem doesn't yet have the required features or is broken, I don't see any other way around changing gem. Until we have something solidly working, I don't think we can afford to optimize for speed.
@Arafatk what about https://github.com/ruby-protobuf/protobuf, it seems to be the most popular protobuf-gem on Rubygems: https://rubygems.org/search?utf8=%E2%9C%93&query=protobuf. Again, I'm not really sure what you're looking for in a protobuf-gem, just wanted to throw it out there.
@chrhansen The main reason why I used ruby protocol buffers instead of google protobuf was because the image recognition model that was saved in the tensorflow_inception_graph.pb could not be read by it and ruby crashed when I tried to do that. Its a 95 mb file and finding the real bug is obviously not easy. So I tried a few other protobuf gems. Luckily ruby protocol buffers worked fine for what I wanted to do.
Comparision For google protobuf
/usr/share/rvm/rubies/ruby-2.2.1/bin/ruby -I/home/arafat/.rvm/gems/ruby-2.2.1/gems/rspec-core-3.4.2/lib:/home/arafat/.rvm/gems/ruby-2.2.1/gems/rspec-support-3.4.1/lib /home/arafat/.rvm/gems/ruby-2.2.1/gems/rspec-core-3.4.2/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb
..............................................................
Finished in 1.86 seconds (files took 11.34 seconds to load)
62 examples, 0 failures
For ruby protocol buffers
/usr/share/rvm/rubies/ruby-2.2.1/bin/ruby -I/home/arafat/.rvm/gems/ruby-2.2.1/gems/rspec-core-3.4.2/lib:/home/arafat/.rvm/gems/ruby-2.2.1/gems/rspec-support-3.4.1/lib /home/arafat/.rvm/gems/ruby-2.2.1/gems/rspec-core-3.4.2/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb
.................................................
Finished in 3.97 seconds (files took 0.36766 seconds to load)
49 examples, 0 failures
I am not sure, if this is the best way to compare speed but it still seems that ruby protocol buffers is kinda slow. I think maybe one long way would be for me to try out the most popular protobuf gems(at least 5) and see if they can actually work (Most of them won't but I don't remember which ones) and then benchmark which would give the best time.
In my opinion the main criteria is a well-tested gem, secondarily speed. If the gem is missing needed features or keeps crashing, speed is irrelevant. As far as I can see the google-protoc is still in alpha, so they don't even claim that it should be ready for real world use.
@chrhansen This is also a good way to think about it and its very important that the gem works properly. So we now have 2 options either I make the changes and merge ruby-protocol buffers directly or I try different gems and see if anyone is faster, if not then we merge ruby protocol buffers?
If it takes only 30min or less to test the most popular gem, then I'd recommend that. If you think it would take more time, then I'd say just merge the ruby-protocol-buffers so we can move on.
I'm not by my laptop now so I can't check myself, but last thing would be to simply confirm that the gem we pick looks to still be maintained (recent commits) and have green CI-labels.
@chrhansen This will take more time. I think that, if I did end up finding a faster gem, I would only have to make changes to core and a few other places before merging therefore it would be good if I were to merge this first. Also we need to have scalar tensor before merging this as I have used it in describing the tensor for file path. After that is done I can make a new pr because there seem to be quite a few merge conflicts here.
@Arafatk ok, so replace the protobuf gem or add scalar + shape first? I'm happy to continue with #60 and then later we can swap out the protobuf-gem, but either way works for me.
@chrhansen Adding Scalar + Shape first would be great and when that happens, it would be easy to replace the protobuf gem and also add the image recognition tutorial.
@Arafatk 👍
@Arafatk I'm currently working on #60 to fix
#constant
(support scalar and shape) and I'm moving code around insideTensorflow::Tensor
. E.g. I'm working onshape_proto
, which currently assumes all tensors are rank 1 or 2 (as far as I can tell). But if we're switching to another gem to read and write protocol buffers, I'll hold off till at least the protobuf-gem has been changed.If we are switching to a new protobuf-gem, would it be possible to update only those changes (gemfile and the stuff in
lib/tensorflow/core
) in a separate PR today, while you polish this image-recognition PR?