One: Compiler Frontend: Operators to enable

[seanshpark]

Welcome to ONE compiler contribution!
Our Compiler frontend is supporting more operators day by day.
This issue is to share our next step Operstors to support and provide a guide how-to.

steps to work

1. select an operator you want to work and create an issue
   
   
   
   • add a link to this issue by adding like "For #277"
   
   
   • use issue title like Compiler FE: Enable ### Op
   
   
2. someone(mostly I will) mark your ID in the table
3. work on a draft code following the modules one by one
4. post draft PR; parial codes of draft PR is OK
5. finish draft and update to PR
   
   
   
   • luci_unit_readtest and luci_unit_writetest should pass with adding to luci/tests
   
   
   • use DRAFT Compiler FE prefix in the title
   
   
   • add 3 labels DRAFT, PR/NO MERGE, PR/NO TEST
   
   
6. post separate PRs one by one with below steps
   
   
   
   1. TensorFlowPythonExamples
   
   
   2. tflite2circle , tflchef , tfldump , circledump : these can be posted in parallel
   
   
   3. (Added) Add item in common-artifacts/exclude.lst
   
   
   4. TensorFlowLiteRecipes
   
   
   5. luci/lang
   
   
   6. luci/import , luci/service , luci/logex , luci/export : these can be posted in parallel
   
   
   7. luci/tests
   
   

reference links

https://github.com/Samsung/ONE/issues?q=is%3Aissue++compiler+enable will show a list of issues used to track Operation support

https://github.com/Samsung/ONE/pulls?q=is%3Apr+DRAFT+Compiler+ will show a list of drafts that are related with Operation support.

[seanshpark]

modules to touch in compiler folder

1. res/TensorFlowLiteRecipes (in top folder)
2. res/TensorFlowPythonExamples (in top folder)
3. tflite2circle
4. tflchef
5. tfldump, circledump
6. luci/lang
7. luci/import
8. luci/service
9. luci/logex
10. luci/export
11. luci/tests : add to test.lst file

[seanshpark]

32 operators to enable

Priority | Operator | Status
-- | -- | --
1 | CAST |  @seanshpark (#486) done
2 | LOGISTIC |  @seanshpark (#537) done
1 | REDUCE_PROD | @seanshpark (#629) done
1 | REDUCE_SUM | @llFreetimell (#628) done
2 | SELECT |  @seanshpark (#732) done
2 | SIGMOID |  @seanshpark (#639) done (== LOGISTIC)
1 | SIN |  @seanshpark (#386) done
2 | SPACE_TO_BATCH_ND |  @seanshpark (#382) done
2 | SPLIT |  @seanshpark (#646) done
1 | SQUARED_DIFFERENCE |  @karthik-pen (#404) done
1 | STRIDED_SLICE |  @seanshpark (#554) done
2 | SUM = (REDUCE_SUM ) |  ~@d-krylov (#429)~
1 | WHILE |  @jinevening (#374) done
2 | FLOOR_MOD | @seanshpark (#819) done
2 | LOGICAL_AND |  @sun-sharma (#406) done
2 | SQUARE |  @kishcs (#405) done
2 | REDUCE_ANY | @seanshpark (#851) done
1 | FILL | @toomuchsalt (#354) done
1 | SQUEEZE | @toomuchsalt (#572) done
1 | ZEROS_LIKE | @toomuchsalt (#358) done
1 | SHAPE |  @d-krylov (#428) --> @seanshpark #1006 done

Priority | Operator | Status
-- | -- | --
1 | EXPAND_DIMS |  @AShedko (#420) done
2 | FLOOR_DIV | @d-krylov (#432 ) done
2 | GREATER |  @d-krylov (#421) done
1 | GREATER_EQUAL |  @d-krylov (#422) done
2 | LESS |  @d-krylov (#423) done
2 | MAXIMUM |  @d-krylov (#424) done
2 | MINIMUM |  @d-krylov (#425) done
1 | NOT_EQUAL |  @d-krylov (#426) done
2 | ONE_HOT |  @struss (#668) done
1 | RANGE |  @kvochko (#427) done
2 | REDUCE_MAX | @d-krylov (#433) done

[seanshpark]

Notice to operator developers;

• order of steps are like (1) post an issue (2) mark the operator with ID and issue number (developers don't need to do this; I'll add when issue is fired) (3) post a draft (4) work in the draft
• I'll clean all the items without any issue or draft in a day or two

[seanshpark]

I've added Priority column to privode which operators to enable first.

[kishcs]

@seanshpark We are not able to add labels (DRAFT, PR/NO MERGE, PR/NO TEST) in DRAFT PR.

/cc @sun-sharma @karthik-pen

[seanshpark]

We are not able to add labels (DRAFT, PR/NO MERGE, PR/NO TEST) in DRAFT PR.

I've added them :)

[kishcs]

I've added them :)

Thank you :)

[ko-dimi]

4. post draft PR; parial codes of draft PR is OK

@seanshpark, I think this step requires clarification. I see some drafts not accepted. Do we require draft to be accepted by reviewer before going further?

[seanshpark]

I see some drafts not accepted.

I actually don't accept (click on "approval" button) draft codes.
I scan the codes but don't thoroughly review codes one by one with the draft.
I may add comments if something is suspicious.

I check on as written above

luci_unit_readtest and luci_unit_writetest should pass with adding to luci/tests

I assume contributors run testing

• to test the pass, tests.lst file should have read and write test with the operator.
• I check this file exist with the operator

Do we require draft to be accepted by reviewer before going further?

Nope.

[ko-dimi]

Do we require draft to be accepted by reviewer before going further?

Nope.

I believe that this is sub-optimal and time consuming way. I suggest we change process to posting one PR with complete draft consisting of 11 commits, one per component. Then review and accepted this draft and then merge with "rebase and merge" button. "Rebase and merge" will allow to have separate commit per component.
This will reduce number of PR for you to review 11 times. Given, that we have to wait each review approximately 24h due to timezone difference, this is quite time saving approach. Do you agree with that approach?

[seanshpark]

I suggest we change process to posting one PR with complete draft consisting of 11 commits, one per component.

This is OK but may consume time in review and if there are any change requests.

"Rebase and merge"

We don't allow this option.

[ko-dimi]

I suggest we change process to posting one PR with complete draft consisting of 11 commits, one per component.

This is OK but may consume time in review and if there are any change requests.
Ok, thank you, I believe this is faster way.
In order to make it even more faster one good trick could be used: you can write in review "OK with this (something) change".

"Rebase and merge"

We don't allow this option.
Could you please explain why?

I want SRR developers to be able to merge code by means of Alexander after you reviewed draft. This is strong requirement because Alexander is in the same timezone and can do merge without 12h wait time.

[binarman]

@seanshpark

I want to clarify several things:
1) Can we merge Draft PR (with one commit per component) after review in one piece?
2) If no - can I merge separate component-related PRs without your approval?

[seanshpark]

Can we merge Draft PR (with one commit per component) after review in one piece?

I'm not sure yet but let's try.

[lemmaa]

I want SRR developers to be able to merge code by means of Alexander after you reviewed draft. This is strong requirement because Alexander is in the same timezone and can do merge without 12h wait time.

Another point of view on this is to find a way for a person to work so that all things are not blocked simultaneously by any single task at any time. In other words, everyone always needs to do things that are independent of each other in parallel. So even if one of them is blocked for one day, two days, or more, it is possible to continue doing other things independently of it. The method will vary from person to person.

It may be difficult to adapt because this is different from the way you used to work in the past. But I think this is the ultimate solution for smooth collaboration from different timezones.

[seanshpark]

compiler/luci/logex/src/FormattedGraph.cpp is missing with

• GREATER
• GREATER_EQUAL

[seanshpark]

after review in one piece?

To review without confusion, for the draft codes, please resolve conflicts.

[toomuchsalt]

@seanshpark I agree that reading code, which is too different from upstream, may be confusing at best. However most of the merge conflicts raise from the operator lists in different parts of the codebase( CircleNodes.lst, CircleNodes.h, OpChef.def, OpChefs.h, GraphBuilderRegistry.cpp )

It's just not viable to do rebases/merges every time new operator is merged, since it's just wasted effort. Other than that( and occasional conflicts in FormattedGraph.cpp and OpPrinter.cpp ) there are no common places which change during operator merges.

Maybe we should consider doing the same thing as in tflchef/proto/tflchef.proto - just predeclare all of operators we want to support and comment out unsupported ones. This should make merge conflicts much less frequent, so only operators located very close would conflict

[seanshpark]

All done for this issue.

[seanshpark]

Thanks for your hard work!!!

[binarman]

Yay, thank you!

[dr-venkman]

Welcome to ONE compiler contribution!
Our Compiler frontend is supporting more operators day by day.
This issue is to share our next step Operstors to support and provide a guide how-to.

steps to work

1. select an operator you want to work and create an issue

   • add a link to this issue by adding like "For #277"
   • use issue title like Compiler FE: Enable ### Op
2. someone(mostly I will) mark your ID in the table
3. work on a draft code following the modules one by one
4. post draft PR; parial codes of draft PR is OK

5. finish draft and update to PR

   • luci_unit_readtest and luci_unit_writetest should pass with adding to luci/tests
   • use DRAFT Compiler FE prefix in the title
   • add 3 labels DRAFT, PR/NO MERGE, PR/NO TEST

6. post separate PRs one by one with below steps

   1. TensorFlowPythonExamples
   2. tflite2circle , tflchef , tfldump , circledump : these can be posted in parallel
   3. TensorFlowLiteRecipes
   4. luci/lang
   5. luci/import , luci/service , luci/logex , luci/export : these can be posted in parallel
   6. luci/tests

reference links

https://github.com/Samsung/ONE/issues?q=is%3Aissue++compiler+enable will show a list of issues used to track Operation support

https://github.com/Samsung/ONE/pulls?q=is%3Apr+DRAFT+Compiler+ will show a list of drafts that are related with Operation support.

I think it would be helpful to mention any additions to common-artifacts here. If an issue pertaining to it does exist, then maybe reference it, especially for point 6.iii. In my specific implementation, I have had to exclude recipes from circle optimization, in addition to generated testcases (please refer pull request #3410 for the details).