Skip to content

Conversation

karmel
Copy link
Contributor

@karmel karmel commented Mar 29, 2018

This script is a work in progress, but we want to have some examples available for the Summit of usin g TensorRT with the official models.

@tfboyd - Take a look and make sure it meets your needs.

@k-w-w - Can you read over for readability and general clarity? Deadline is tight, but we can always do follow-up changes in another PR.

@karmel karmel requested review from tfboyd and k-w-w March 29, 2018 20:46
Copy link
Contributor

@k-w-w k-w-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still going through the code. So far it looks very clean and readable!

already, and add the Official Models to your Python path:

```
cd /path/to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove cd /path/to

Might just be me, but I a bit confused by the missing noun at the end. The /path/to/models in the third line is clear on its own.


### Step 3: Get an image to test

The script can accept a JPEG image file to use for predictions. If none is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any requirements for the image, other than the JPEG format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, assuming that we all assume that the picture should have something identifiable and worth labeling in it.


1. [Install TensorFlow.](https://www.tensorflow.org/install/)
2. [Install TensorRT.](http://docs.nvidia.com/deeplearning/sdk/tensorrt-install-guide/index.html)
3. We use the [Official version of ResNet image preprocessing](/official/resnet/imagenet_preprocessing.py). Please checkout the Models repository if you haven't
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Link text is quite long, I would recommend changing "We use the Official version of ResNet image preprocessing" to "We use image preprocessing functions from the Official version of Resnet"

### Step 4: Run the model

You have TensorFlow, TensorRT, a graph def, and a picture.
Now it's time to time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"time to time" nice 👍

input_map={input_node: iterator.get_next()},
return_elements=output_nodes
)
output = outputs[0].outputs[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of 'output' in one line... what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gooood question. As it turns out, it removes the return values of all the passed "output_nodes" except for the first. That seemed unclear, so I fixed the script to accept only one output node, and added comments here, rather than silently dropping the others.


graph = tf.Graph()
with tf.Session(graph=graph) as sess:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line

@karmel
Copy link
Contributor Author

karmel commented Mar 29, 2018

Thanks-- first set of comments much appreciated and addressed.

@karmel
Copy link
Contributor Author

karmel commented Mar 30, 2018

I'm going to merge this now so that we have it ready for tomorrow, but don't hesitate to add more comments; I can fix in separate PRs.

@karmel karmel merged commit 430811a into master Mar 30, 2018
@karmel karmel deleted the feat/trt branch March 30, 2018 01:05
Copy link
Contributor

@k-w-w k-w-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more comments. Perhaps we should add a util function in the official models to directly export a frozen graph.

# Run the graph in various precision modes.
################################################################################
def get_gpu_config():
"""Share GPU memory between image preprocessing and inference."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong, but the image preprocessing and inference sessions don't run at the same time. Is there a reason per_process_gpu_memory_fraction is set to .5?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it is because we reserve the memory for each session, and that must not get cleared. I am not sure, but I tried without that part, and got CUDA malloc errors.

used for statistic calculation
batch_size: int, number of examples per batch

Returns:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does log_stats need to return anything?

@karmel
Copy link
Contributor Author

karmel commented Mar 30, 2018

Thanks for the review on short notice-- frozen graphs, as I understand the story, are not generally encouraged other than a few special use-cases, so probably best not to include by default for now; I think TFLite is planning of having a replacement format at some point in the future as well.

omegafragger pushed a commit to omegafragger/models that referenced this pull request May 15, 2018
* Adding scripts for TensorRT

* Fixing README link

* Responding to CR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants