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

Draft-v2: Implement Circle+ Loader #12045

Closed
wants to merge 7 commits into from

Conversation

zetwhite
Copy link
Contributor

@zetwhite zetwhite commented Nov 16, 2023

related issue : #11692

ONE-DCO-1.0-Signed-off-by: SeungHui Youn sseung.youn@samsung.com

@zetwhite zetwhite force-pushed the 1101/circle-x-importer-v2 branch 5 times, most recently from 3d140cd to ac17f9a Compare November 16, 2023 02:35
tmp

ONE-DCO-1.0-Signed-off-by: SeungHui Youn <sseung.youn@samsung.com>
@zetwhite

This comment was marked as resolved.

@@ -23,6 +23,7 @@ option(BUILD_ONERT_RUN "Build onert_run" ON)
option(BUILD_ONERT_TRAIN "Build onert_train" ON)
option(BUILD_TFLITE_LOADER "Build TensorFlow Lite loader" ON)
option(BUILD_CIRCLE_LOADER "Build circle loader" ON)
option(BUILD_TRAININFO_LOADER "Build circle traininfo loader" ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using circlr+ word instead of traininfo ? Because this pr is for loading circle+ file format. :)

Copy link
Contributor Author

@zetwhite zetwhite Nov 21, 2023

Choose a reason for hiding this comment

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

I intentionally choose the word traininfo instead of circle+.

Because circle+ range is a bit ambiguous.
What is circle+ exactly range?

  • (1) both circle format and train info
  • (2) only train info

I thought circle+ means (1).
But this loader mainly focuses on loading train info from metadata.
So, That's why I choose traininfo loader.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought circle+ contains all meta information included in circle+ file format.
This circle+ loader loads all meta information from circle+ file. And each function can select the meta data it is interested in.

Copy link
Contributor Author

@zetwhite zetwhite Nov 21, 2023

Choose a reason for hiding this comment

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

Oh, I see your point.

But still for me, the words "circle+ loader" look like it is responsible for loading both circle(subgraphs and tensors) and its metadata(train info and .. etc).

Therefore, How about the word circletr_meta_loader ?
+) I also thought the word circle+_meta_loader, but I'm afraid that string "+" cause trouble later.

@@ -25,7 +25,7 @@ namespace onert
{
namespace circle_loader
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use different namespace for circle+ format.

Comment on lines +281 to +287
auto is_given = [&vm](const std::string &s) -> bool { return not vm[s].defaulted(); };

if (is_given("batch_size") && is_given("learning_rate") && is_given("loss") &&
is_given("optimizer"))
{
_is_training_info = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to check if there is a given parameter in main function instead of using _is_training_info variable.

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 agree.

But for now, It is hard to check in the main.
Because Arg has a default value, It is hard to figure out whether the value is given or default in main.

("epoch", po::value<int>()->default_value(5)->notifier([&](const auto &v) { _epoch = v; }), "Epoch number (default: 5)")
("batch_size", po::value<int>()->default_value(32)->notifier([&](const auto &v) { _batch_size = v; }), "Batch size (default: 32)")
("learning_rate", po::value<float>()->default_value(0.001)->notifier([&](const auto &v) { _learning_rate = v; }), "Learning rate (default: 0.001)")
("loss", po::value<int>()->default_value(0)->notifier([&] (const auto &v) { _loss_type = v; }),
"Loss type\n"
"0: MEAN_SQUARED_ERROR (default)\n"
"1: CATEGORICAL_CROSSENTROPY\n")
("optimizer", po::value<int>()->default_value(0)->notifier([&] (const auto &v) { _optimizer_type = v; }),
"Optimizer type\n"
"0: SGD (default)\n"
"1: Adam\n")

@glistening
Copy link
Contributor

glistening commented Nov 21, 2023

@zetwhite Here are some points what I've talked you offline:

  1. I would like to remove training info's default values from several difference places.
    (e.g. flatbuffers, command-line, API structure default, ...)
    It would be good to keep just 1 place (which is reasonable) or no place.
    For example, I think command-line tool (e.g. onert-train specifies training info to override default value, it may be good to remove all default values from command-line tool.

  2. Training information timing needs to be determined.
    While the model itself is loaded during nnfw_load_..., training info was constructed in nnfw_train_prepare. Depending on this decision, the implementation will be changed.

  3. Where to put circle_traininginfo.fbs and its generated file.
    I would like to create circlex_schema (or circle+_schema in the same level of circle_schema.).

@zetwhite
Copy link
Contributor Author

@zetwhite Here are some points what I've talked you offline:

Thanks for the summary.

I found out how-to-append the loader on the current implementation wasn't clear. 😅
I'd better clarify the points and ask circle+'s member opinions offline.

@zetwhite zetwhite requested review from jyoungyun and removed request for jyoungyun November 22, 2023 01:57
Comment on lines +194 to +195
if (model_type == "circle+")
return onert::circle_loader::loadModel(filename.c_str(), /*load metadata*/ true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(notes)

namespace traininfo_loader
{

std::unique_ptr<ir::train::TrainingInfo> loadTrainingInfo(std::shared_ptr<const ir::Model> model);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(notes)

@zetwhite zetwhite closed this Feb 7, 2024
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