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.

Sign up for GitHub

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

Jump to bottom

[Suggestion] add support to use frozen graph #12

Closed
natxopedreira opened this issue Sep 13, 2021 · 17 comments
Closed

[Suggestion] add support to use frozen graph #12

natxopedreira opened this issue Sep 13, 2021 · 17 comments
Labels
feature New feature or request

Comments

@natxopedreira
Copy link
Contributor

Hello!!

What do you think to add support to use frozen graph? That way you can use a pb created in tf1, so is more compatible.... i'm doing that and works ok. Old cppflow had that option
@danomatika
Copy link
Member

danomatika commented Sep 13, 2021 via email

@natxopedreira
Copy link
Contributor Author

Sorry i don't expressed myself ok.

That feature was not on ccpflow2 but was on cppflow1, i added a pair of methods to cppflow2 and one to ofxTF2Model and now i can use a tf1 frozen graph with your addon.

So i just asking if you can consider to add that feature as it makes more "open" as you are not restricted to savedmodel format.

Its only a suggestion

@danomatika
Copy link
Member

danomatika commented Sep 13, 2021 via email

@natxopedreira
Copy link
Contributor Author

exactly, in fact i see there is a pending pr to cppflow that also do that

@bytosaur
Copy link
Member

Hey @natxopedreira,
That is in fact a very cool feature and we ll happily review and accept a PR :)

@natxopedreira
Copy link
Contributor Author

Great! Will prepare it and upload to my fork

@natxopedreira
Copy link
Contributor Author

I have a question about the preffered style to use.

I need to add a second constructor to cppflow model class to have one constructor to use with frozen and another to savedmodel so both will have the url string as parameter, right now as im in a hurry i added a dummy second parameter to the constructor of frozen and done, but i want to do it ok to send the pr.

I can for example on the "frozen" constructor send TF_Buffer as parameter instead the string with the path or a file...

Ideas on how to deal with the second constructor?

I dont know if im explaining myself ok :)

@danomatika
Copy link
Member

Ideas on how to deal with the second constructor?

I dont know if im explaining myself ok :)

Can you add pseudo code here via Markdown?

@natxopedreira
Copy link
Contributor Author

natxopedreira commented Sep 15, 2021 •
edited by bytosaur

Sure, here is what im using, you can see the ugly hack of adding the dummy second parameter to model constructor

model.h

inline TF_Buffer *model::readGraph(const std::string& filename) {
        std::ifstream file (filename, std::ios::binary | std::ios::ate);

        // Error opening the file
        if (!file.is_open()) {
            std::cerr << "Unable to open file: " << filename << std::endl;
            return nullptr;
        }


        // Cursor is at the end to get size
        auto size = file.tellg();
        // Move cursor to the beginning
        file.seekg (0, std::ios::beg);

        // Read
        auto data = std::make_unique<char[]>(size);
        file.seekg (0, std::ios::beg);
        file.read (data.get(), size);

        // Error reading the file
        if (!file) {
            std::cerr << "Unable to read the full file: " << filename << std::endl;
            return nullptr;
        }


        // Create tensorflow buffer from read data
        TF_Buffer* buffer = TF_NewBufferFromString(data.get(), size);

        // Close file and remove data
        file.close();

        return buffer;
    }

    inline model::model(const std::string &filename, float p) {
        this->graph = {TF_NewGraph(), TF_DeleteGraph};

        auto session_deleter = [](TF_Session* sess) {
            TF_DeleteSession(sess, context::get_status());
            status_check(context::get_status());
        };


        std::unique_ptr<TF_SessionOptions, decltype(&TF_DeleteSessionOptions)> sess_opts = {TF_NewSessionOptions(), TF_DeleteSessionOptions};

        this->session = {TF_NewSession(this->graph.get(), sess_opts.get(), context::get_status()),session_deleter};
        status_check(context::get_status());

        // Import the graph definition
        TF_Buffer* def = readGraph(filename);
        if(def == nullptr) {
            throw std::runtime_error("Failed to import gragh def from file");
        }

        std::unique_ptr<TF_ImportGraphDefOptions, decltype(&TF_DeleteImportGraphDefOptions)> graph_opts = {TF_NewImportGraphDefOptions(), TF_DeleteImportGraphDefOptions};
        TF_GraphImportGraphDef(this->graph.get(), def, graph_opts.get(), context::get_status());
        TF_DeleteBuffer(def);

        status_check(context::get_status());
    }

ofxTF2Model.h

bool Model::loadGraph(const std::string & modelPath) {
	Model::clear();
	auto model = new cppflow::model(modelPath,0.5);
	if(!model) {
		modelPath_ = "";
		ofLogError("ofxTensorFlow2") << "Model: model could not be initialized!";
		return false;
	}	
	model_ = model;
	modelPath_ = modelPath;
	ofLogVerbose("ofxTensorFlow2") << "Model: loaded model: " << modelPath_;
	return true;
}

@bytosaur
Copy link
Member

bytosaur commented Sep 15, 2021 •
edited

First of all thanks for your contribution!

If it's a cppflow thing than I think we need to wait for the original PR to happen. I think they did a really good job and maybe they have their reasons for not accepting it yet...

in the meantime we could create a branch that pulls from the fork where the PR comes from and merge your edits to the Model class.

@natxopedreira
Copy link
Contributor Author

super!!!! but @bytosaur that take a look at the code in pr first.

  1. There is code that is not need as is taken directly from the actual constructor and not related in this case.

  2. They take code from ccpflow 1 and seting the config_options for session no longer works that way, it will no crash but those will not work, i for example spend hours trying to enable gpu percentage ussage as those options suggested on cppflow1 and the silent fail... and finally find that is not need as current implementation in cppflow2 works

I will suggest make your own branch, use the code cleaned and wait if they solved/merge that one

@bytosaur
Copy link
Member

Right, it reminded me of the code for the GPU settings which is not related to the model. I dont think it should belong there even if it works.

so, yeah it s probably best to fork cppflow and edit it on our own.

@bytosaur bytosaur added the feature New feature or request label Sep 15, 2021
@natxopedreira
Copy link
Contributor Author

no it dont work, you dont get any error but no longer works....

@bytosaur
Copy link
Member

hey @natxopedreira,

I have forked and modified the model class of cppflow here

I have added a branch containing changes:

the model can be downloaded here

let me know what you think :) and thanks for the support

@bytosaur
Copy link
Member

bytosaur commented Sep 15, 2021 •
edited

argh! i keep getting problems with those submodules...

this solved it however

[main] $ git submodule deinit
[main] $ git checkout 'new_branch'
[new_branch] $ git submodule update --init --recursive

@natxopedreira
Copy link
Contributor Author

I like it very much! great solution with the enum!!! and i think having support for frozen graph is super cool as you can use tf1 models. I think you should add a note on readme about that

Thanks!!!

@danomatika
Copy link
Member

danomatika commented Sep 16, 2021 via email

@bytosaur bytosaur mentioned this issue Sep 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@danomatika @natxopedreira @bytosaur