The Wayback Machine - https://web.archive.org/web/20200917115805/https://github.com/tensorflow/model-optimization/pull/531
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

Add support for tf.distribute after enabling the update of cluster indices #531

Open
wants to merge 2 commits into
base: master
from

Conversation

@Ruomei
Copy link
Collaborator

Ruomei commented Sep 4, 2020

This PR adds the support for tf.distribute after enabling the update of cluster indices. Since these variables are deterministic (they depend on weights but not batch inputs), they are set as SyncOnRead so that they are calculated in each replica independently. In the model exporting time, they will be read from the first replica. Hope in this way we can avoid unnecessary sync for large LUTs.

This PR should follow #519, and it tests the change in cluster indices in cross-replica context (in the unit test). That is why it contains two commits.

@googlebot googlebot added the cla: yes label Sep 4, 2020
@haozha111 haozha111 requested a review from akarmi Sep 11, 2020
Copy link
Collaborator

akarmi left a comment

Thank you. Just a nit from me. @wwwind, could you please have a look at this change as well?


def _distribution_strategies():
return [
# tf.distribute.experimental.MultiWorkerMirroredStrategy(),

This comment has been minimized.

@akarmi

akarmi Sep 14, 2020 Collaborator

Please, leave in only the strategies we want to be testing here, and remove unnecessary commented code here and in other places below.

This comment has been minimized.

@Ruomei

Ruomei Sep 16, 2020 Author Collaborator

Thanks, done.

@akarmi akarmi requested a review from wwwind Sep 14, 2020
for ct, weight in enumerate(self.layer.weights):
name = self._weight_name(weight.name)
full_name = self.layer.name + "/" + name
full_name = '{}{}{}'.format(self.layer.name, '/', name)

This comment has been minimized.

@wwwind

wwwind Sep 14, 2020 Collaborator

the character '/' could be inside of the string

This comment has been minimized.

@Ruomei

Ruomei Sep 16, 2020 Author Collaborator

Thanks, done.

@wwwind
wwwind approved these changes Sep 14, 2020
Copy link
Collaborator

wwwind left a comment

looks good

@Ruomei Ruomei force-pushed the Ruomei:toupstream/distribution_cluster_indices branch from 6c5fe39 to b72e2ce Sep 16, 2020
@akarmi
Copy link
Collaborator

akarmi commented Sep 17, 2020

Thank you. Just waiting for #519 to land first, before merging this.

@akarmi
akarmi approved these changes Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.