[Runtime] Pipeline Executor Second patch, configuration load and executor export/import. by huajsj · Pull Request #9108 · apache/tvm

huajsj · 2021-09-24T21:00:56Z

This patch is one of serial patch for PR 7892 splitting. this is second part the pipeline executor, include the configuration JSON file load and pipeline executor export and import.

huajsj · 2021-09-24T23:50:25Z

@comaniac @masahi , please take a look.

huajsj · 2021-09-27T18:29:58Z

@comaniac.

comaniac · 2021-09-27T18:40:04Z

Probably don't have many bandwidths this week to review this PR but I'll try.
cc @masahi to see if there's any chance.

masahi · 2021-09-28T12:50:10Z

I'll take a look tomorrow.

masahi

Did the first pass, still too many basic grammar errors that distract me from focusing on technical details.

I don't want to correct all of them, so please go through ALL of your change again:

Fix as many grammar mistake as possible. Things like the lack of a, the or s for plural, third person pronoun (this function return -> this function returns).
Variable names should be snake_case
You use the term configure as if it was a noun. Replace all of them with config.

masahi

There are still many grammar and other minor errors. Please go through them again. Correcting only the ones I pointed out is not sufficient.

Remove all _configure names as I said in the previous review.

In many places you use an index subtracted by 1, like string_config[mod_idx - 1] = mconf. I don't like them. Please restructure your code so that such weird indexing is unnecessary.

masahi · 2021-10-04T10:06:04Z

+ reader->Read(&input_name);
+ }
+ }
+ ICHECK(mod_idx >= 0);


So this mod_idx can be zero while others are > 0? I think it is confusing.

in module config file, there are 2 place have mod_idx information , first is the module list with output, here all module is graphexecutor module, and no PipelineExecutor or global module here, the second place is the binding place that use to describe which output binding with which module input/output, here the module can be graphexecutor module or PipelineExecutor/global module, because we need import data or output data for PipelineExecutor, that is the reason why in different place there are difference check logic.
added related comments for the said logic explain.

masahi · 2021-10-04T10:14:01Z

@huajsj Why mod_index etc starts from 1? What does index 0 corresponds to? Unless there is a good reason for this weird design, please make all indices start from 0.

huajsj · 2021-10-12T22:53:58Z

Your latest commit cdfa5c2 is full of grammar issues. Please take a look again.

fixed. @masahi , please take a look.

masahi · 2021-10-13T00:06:01Z

+ '"load_config" or "pipeline_config" is missing in %s' % config_file_name
+ )
+
+ # The config file use to load library, prameters, and JSON files.


masahi · 2021-10-14T00:35:59Z

thanks @huajsj @comaniac

…utor export/import. (apache#9108) * [pipeline executor] Add configuration load function and pipeline executor export,import function. * address review comments. * polish comments and doc string. * address review comments. * address review comments. * Change mod_idx start from 0, remove mod_idx - 1 logic. * address review comments. * polish documents. * adress review comments * address review comments. * address review comments. * polish the document. * address review comments. * address review comments. * polish comments. * Triger build. * address review comments. * address review comments. * fix grammar issue. * polish documents. * add single global binding check. * address review comments. * trigger build.

huajsj requested review from ZihengJiang, areusch, comaniac, jroesch, junrushao, kazum, liangfu, masahi, merrymercy, tmoreau89, tqchen, vinx13, yzhliu and zhiics as code owners September 24, 2021 21:00

masahi requested changes Sep 28, 2021
View reviewed changes

huajsj requested a review from masahi September 30, 2021 00:56

masahi requested changes Sep 30, 2021
View reviewed changes

huajsj force-pushed the small branch from 15c5e58 to 3130cb4Compare October 1, 2021 07:32

masahi reviewed Oct 4, 2021
View reviewed changes

Comment threadsrc/runtime/pipeline/pipeline_executor.h Outdated

masahi reviewed Oct 4, 2021
View reviewed changes

Comment threadsrc/runtime/pipeline/pipeline_executor.h Outdated

masahi reviewed Oct 4, 2021
View reviewed changes

Comment threadsrc/runtime/pipeline/pipeline_function.cc Outdated

masahi reviewed Oct 4, 2021
View reviewed changes

Comment threadsrc/runtime/pipeline/pipeline_function.cc Outdated

masahi reviewed Oct 4, 2021
View reviewed changes

Comment threadsrc/runtime/pipeline/pipeline_function.h Outdated

masahi reviewed Oct 4, 2021
View reviewed changes

Comment threadsrc/runtime/pipeline/pipeline_struct.h Outdated

fix grammar issue.
15a8f9f

huajsj requested a review from masahi October 12, 2021 01:20

huajsj added 2 commits October 11, 2021 22:43

polish documents.
5e55c8e

add single global binding check.
b6cf7ab

masahi reviewed Oct 12, 2021
View reviewed changes

Comment threadsrc/runtime/pipeline/pipeline_struct.h Outdated

masahi reviewed Oct 12, 2021
View reviewed changes

Comment threadsrc/runtime/pipeline/pipeline_executor.h Outdated