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

[WIP][Network] fix canExecuteInPlace logic #2790

Closed
wants to merge 1 commit into from

Conversation

baek2sm
Copy link
Contributor

@baek2sm baek2sm commented Nov 8, 2024

currenly, there is an issue where in-place might not be applied even if you set supportInplace to true in the layer-node. Regarding this, I think that there is a issue with the "canExecuteInPlace" function which determines the type of in-place.

The logic of this function is as follows:

  • step 1. If the supportInPlace property of the layer node is set to false, in-place is disabled.
  • step 2. If the supportInPlace property of the layer node is set to true, decide whether to execute restricting_inplace or non_restricting_inplace through various conditional statements.
  • step 3. If they do not meet "step 2"'s conditions, in-place is disabled.

However, even though supportInPlace is set to true in layerNode, there could be situations where it doesn't satisfy the requirements mentioned in "step 2".

For example, in the condition "if there is no operation in the layer or it does not support backwarding," it decides whether to execute restricting_inplace or non_restricting_inplace depending on the connection relationship between the layers.

However, if an "add layer" is added, since the layer has operations and supports backwarding, it does not correspond to the above conditions at all, so even if supportInPlace is set to true, in-place may not set to restricting_inplace or non_restricting_inplace.

So I suggest modifying "step 3" as follows: "If they do not meet "step 2"'s conditions, non-restricting-inplace is enabled."
Wouldn't it be better to add "step 2" only when exception handling is required?

In summary, at the last step of this function, since the in-place option is turned on and it does not meet the conditions where inplace needs to be forcibly turned off or a restricting-inplace operation should be performed, I suggest to execute non-restricting-inplace in this case.

Self evaluation:

  1. Build test: [X]Passed [ ]Failed [ ]Skipped
  2. Run test: [X]Passed [ ]Failed [ ]Skipped

currenly, there is an issue where in-place might not be applied even if you set supportInplace to true in the layer-node.
Regarding this, I think that there is a issue with the "canExecuteInPlace" function which determines the type of in-place.

The logic of this function is as follows:
step 1. If the supportInPlace property of the layer node is set to false, in-place is disabled.
step 2. If the supportInPlace property of the layer node is set to true, decide whether to execute restricting_inplace or non_restricting_inplace through various conditional statements.
step 3. If they do not meet "nnstreamer#2"'s conditions, in-place is disabled.

However, even though supportInPlace is set to true in layerNode, there could be situations where it doesn't satisfy the requirements mentioned in "step 2".

For example, in the condition "if there is no operation in the layer or it does not support backwarding," it decides whether to execute restricting_inplace or non_restricting_inplace depending on the connection relationship between the layers.

However, if an "add layer" is added, since the layer has operations and supports backwarding, it does not correspond to the above conditions at all, so even if in-place is set to true, in-place may not work.

So I suggest modifying "step 3" as follows: "If they do not meet "step 2"'s conditions, non-restricting-inplace is enabled."
Wouldn't it be better to add "step 2" only when exception handling is required?

In summary, at the last step of this function, since the in-place option is turned on and it does not meet the conditions where inplace needs to be forcibly turned off or a restricting-inplace operation should be performed, I suggest to execute non-restricting-inplace in this case.

**Self evaluation:**
1. Build test:   [X]Passed [ ]Failed [ ]Skipped
2. Run test:     [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Seungbaek Hong <[email protected]>
@taos-ci
Copy link

taos-ci commented Nov 8, 2024

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2790. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

Copy link

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@baek2sm, 💯 All CI checkers are successfully verified. Thanks.

@jijoongmoon
Copy link
Collaborator

Would you be able to handle the in-place condition in the layer itself? Currently there is condition check in netowrk_graph depending on the layertype.

auto no_op = [](const std::shared_ptr<LayerNode> &lnode) {

@baek2sm
Copy link
Contributor Author

baek2sm commented Nov 11, 2024

@jijoongmoon This modification is being made so that in-place operations can be handled in the layer itself.

Of course, adding the name of the layer type to the no_op function in the network_graph will cause the corresponding layer to behave as in-place. However, this completely contradicts the purpose for which the no_op function was created.

The purpose of the no_op function is stated as follows: "If the layer is a no-op, it can perform in-place operations since it does not modify its input/output tensors and does not require checking its neighboring nodes for dependencies." Currently, both the flatten and identity layers have been added here.. Do you want to list all layer types here for support of in-place operations?
auto no_op = [](const std::shared_ptr<LayerNode> &lnode) { return lnode->getType() == FlattenLayer::type || lnode->getType() == IdentityLayer::type; };

Please remember that the condition for this part of code to execute is when supportInplace is set to true in the layer, already.
But this commit is not the best, too. So I've changed status to WIP in order to improve non-restrictive and restrictive conditions.

@baek2sm baek2sm changed the title [Network] fix canExecuteInPlace logic [WIP][Network] fix canExecuteInPlace logic Nov 11, 2024
@baek2sm baek2sm added the WIP label Nov 11, 2024
@baek2sm
Copy link
Contributor Author

baek2sm commented Nov 11, 2024

I will organize it with other commit to improve in-place system and upload it as a new PR.

@baek2sm baek2sm closed this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants