Fix a bug in CachedOP.

[zheng-da]

Description

As reported in #12116, the modification in #11951 causes excessive memory consumption. The reason is that #11951 actually turns on recording even in the inference mode.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

• The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
• Changes are complete (i.e. I finished coding on this PR)
• All changes have test coverage:
• Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
• Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
• Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
• Code is well-documented:
• For user-facing API changes, API doc string has been updated.
• For new C++ functions in header files, their functionalities and arguments are documented.
• For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
• Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
• To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

• Feature1, tests, (and when applicable, API doc)
• Feature2, tests, (and when applicable, API doc)

Comments

• If this change is a backward incompatible change, why must this change be made.
• Interesting edge cases to note here

[zheng-da]

@piiswrong

[safrooze]

I recommend putting the recording logic in this line and not changing the semantics of recording variable in case in the future someone decides to use recording below RunGraph call.

  RunGraph(false, idx, arrays, 0, idx.num_nodes(), std::move(array_reqs),
           std::move(ref_count), &states, dispatch_modes,
           recording && inlining_);

[zheng-da]

i'm trying to follow the logic of the original code. A little logic error can cause unexpected problems.

[safrooze]

I find it easier to understand the logic: RunGraph must record only when autograd is recording and CachedOp is inlining. An easier to understand logic can prevent future confusion. Up to you to change or not.

[safrooze]

I recommend clarifying in the comment that "When CachedOp is inlining, RunGraph is in charge of recording the graph, otherwise we are in charge of recording and RunGraph shall not be recording".

[safrooze]

Do we have any unit-test framework to check the growth of the bufferpool that we can use to write a unit-test for this fix?

[safrooze]

Tested the fix with my code and the excessive memory allocation issue reported in #12116 is fixed. Would be great to have a unit-test to avoid future regressions.

[haojin2]

@eric-haibin-lin @piiswrong This PR should be good to merge?