It seems that we need this fix because the last parameter of TensorCopy has a default value:
The code style does warned the use of default argument: https://google.github.io/styleguide/cppguide.html#Default_Arguments
In short, the existence of this default argument here might make users ignore the fact that TensorCopy has two modes, thus leads to some mis-uses.
I would suggest to slightly change this PR to
- remove the default argument from
TensorCopy
, - add a
TensorCopySync
, - change all the calls to
TensorCopy(..., true)
intoTensorCopySync(...)
, and - mark this PR as
fix #10242
.
Write the test before write the feature
One step forward: Create a PR just contains the test, then make that PR pass CI.
- Example: Write OpTest in C++ rather than Python. If we can test operators in cpp, then don't involve Python.
- Pros:
- Avoid exposing Scope, Operator to Python. (less code, less bug)
- Avoid dependency on Executor and Backward
- Cons:
- Python does have some handy packages like numpy, pdb.
- Pros:
For an individual, before adding a new feature (SelectedRow, ParallelExecutor, Memory Optimizer etc), write a design doc for it; before workingon an PR, write an issue for it. Don't start writing a single line of code if you can't put your idea into words. Becuase it is likely that you don't understand where you code will end up. Without a description of your purpose, you prevent others from helping you.
I didn't write a design doc for ParallelDo
at the first place. Even plenty of my coworkers reminded me of writing one, I tried to avoid it because I have "more important" things to do, i.e. writing code. As a result, three MOUNTHs of effort in developing ParallelDo
are in vain. It is replaced by ParallelExecutor
.
Looking back, I could have put more thoughts on the design, discussed it more with other core members and gave directly feedback to the tech lead. A well-written design doc can solve all these "could-have".
Harry Shum has a nice article elaborating of the importance of the writing.