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

Added feature for serialized session options #28

Merged
merged 2 commits into from
May 31, 2020

Conversation

AfaqSabirIBEX
Copy link
Contributor

Firstly thank you for creating this library, it has helped us a lot in development.

Recently we have moved to using Tensorflow on the GPU but to get this working properly along side other software we need to be able to modify TF session options, so that we can control the amount of memory TF uses. This PR allows the user to input options if needed when the model is loaded.

@ljn917
Copy link
Contributor

ljn917 commented May 28, 2020

Hi, it seems TF_CreateConfig() is a better solution for your need.

@serizba
Copy link
Owner

serizba commented May 29, 2020

Hi @AfaqSabirIBEX

I think is a great idea and it can be useful. In #3 I answered how to use TF_SetConfig, but I did not add the feature to the main code, I don't know why.

I would suggest you to add a bit of documentation (an example, or maybe in the readme) on how to fill the config_options vector, as it can be a bit tricky. It would be great if you can add it 😊

Thanks!

@AfaqSabirIBEX
Copy link
Contributor Author

Hi @AfaqSabirIBEX

I think is a great idea and it can be useful. In #3 I answered how to use TF_SetConfig, but I did not add the feature to the main code, I don't know why.

I would suggest you to add a bit of documentation (an example, or maybe in the readme) on how to fill the config_options vector, as it can be a bit tricky. It would be great if you can add it 😊

Thanks!

Hi @serizba

Thanks for the feedback. I have updated the load_model example and added some comments to main.cpp. I also added a script to show how to generate the serialized config options and added a single comment to the model.h to point user in the correct direction.

I figured updating the load_model example is probably the cleanest thing to do and does not over complicate this change. Please let me know if this is correct or any more changes are required, as we are eager to start using this feature in our code.

Thanks again

@AfaqSabirIBEX
Copy link
Contributor Author

Hi, it seems TF_CreateConfig() is a better solution for your need.

Hi @ljn917,

There are a few reason we did not go down the route of using that function:

  • I wanted to keep this change as simple as possible and not include more dependencies.
  • Using that function still does not give the desired result as you cannot set the memory fraction. We want to limit TF GPU memory but allow for growth as well. This allows us to keep TF alive whilst running other code and prevents TF from doing unnecessary memory allocations.
  • We are not able to use anything experimental in our software.

@ljn917
Copy link
Contributor

ljn917 commented May 29, 2020 via email

@serizba serizba merged commit 269f0c6 into serizba:master May 31, 2020
@serizba
Copy link
Owner

serizba commented May 31, 2020

Hi @AfaqSabirIBEX
I think is a great idea and it can be useful. In #3 I answered how to use TF_SetConfig, but I did not add the feature to the main code, I don't know why.
I would suggest you to add a bit of documentation (an example, or maybe in the readme) on how to fill the config_options vector, as it can be a bit tricky. It would be great if you can add it blush
Thanks!

Hi @serizba

Thanks for the feedback. I have updated the load_model example and added some comments to main.cpp. I also added a script to show how to generate the serialized config options and added a single comment to the model.h to point user in the correct direction.

I figured updating the load_model example is probably the cleanest thing to do and does not over complicate this change. Please let me know if this is correct or any more changes are required, as we are eager to start using this feature in our code.

Thanks again

Thanks @AfaqSabirIBEX ! That's perfect. Before merging it I just changed the script to create the serialized options to output a C++ like vector so it is easy to copy-paste directly to the cpp file.

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants