Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Add dedup flag to master from #19112 #19246

Merged
merged 4 commits into from
Oct 1, 2020

Conversation

samskalicky
Copy link
Contributor

@samskalicky samskalicky commented Sep 28, 2020

Description

Added option to disable deduping the subgraph inputs/outputs. @bgawrych asked for this while they migrate the MKLDNN quantization flow from using executor to optimize_for.

Original PR for subgraph deduplication on master #16131. When backporting to 1.x we added the option to enable deduplicating subgraph inputs/outputs in #19112. On master the option will be enabled by default, and the flag will allow disabling as needed.

@mxnet-bot
Copy link

Hey @samskalicky , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [edge, clang, website, centos-cpu, sanity, windows-gpu, centos-gpu, windows-cpu, unix-gpu, miscellaneous, unix-cpu]


Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

Copy link
Contributor

@mseth10 mseth10 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bgawrych bgawrych left a comment

Choose a reason for hiding this comment

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

LGTM, thanks
@anko-intel @pengzhao-intel Maybe you want to take a look too

@samskalicky samskalicky added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Sep 30, 2020
@samskalicky samskalicky merged commit 5dc6cad into apache:master Oct 1, 2020
@grygielski
Copy link
Contributor

@samskalicky I've got a question about the default value of dedup_subgraph flag. It is set inside the constructor to true. However if user does not provide {'dedup_subgraph': 'True'} to option_map, it is set to false anyway. Is it desired outcome? It looks like a bug to me.
https://github.com/apache/incubator-mxnet/blob/0514233103baff5e1581cf2057f561f7a36616c2/src/operator/subgraph/subgraph_property.h#L271-L279

@samskalicky
Copy link
Contributor Author

@samskalicky I've got a question about the default value of dedup_subgraph flag. It is set inside the constructor to true. However if user does not provide {'dedup_subgraph': 'True'} to option_map, it is set to false anyway. Is it desired outcome? It looks like a bug to me.
https://github.com/apache/incubator-mxnet/blob/0514233103baff5e1581cf2057f561f7a36616c2/src/operator/subgraph/subgraph_property.h#L271-L279

Thanks @grygielski for pointing this out. This does seem to be the default case. However, for the subgraph property that is used in the optimize_for flow it does not implement this logic in its PrePartition function:
https://github.com/apache/incubator-mxnet/blob/0faecf01eddc781793927cb7cb9415310e9b21a2/src/operator/subgraph/partitioner/custom_subgraph_property.h#L191-L195
So while the default case for the parent function of the SubgraphProperty class does this logic, subclasses that implement their own PrePartition function can override this behavior as needed.

I should probably clarify that "being enabled by default" applies to the optimize_for flow, rather than all subgraph properties being enabled by default. Sorry for the confusion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants