Skip to content

Conversation

SaoirseARM
Copy link
Contributor

This PR fixes a bug in strip_clustering() where cluster nodes remained in the model when exporting to saved_model format.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no PR contributor has not signed CLA label Jun 10, 2020
@SaoirseARM
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes PR contributor has signed CLA and removed cla: no PR contributor has not signed CLA labels Jun 11, 2020
@akarmi
Copy link
Contributor

akarmi commented Jun 16, 2020

@alanchiao, can you please check this change and merge it if it looks OK to you.

@alanchiao alanchiao added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Jun 16, 2020
@alanchiao
Copy link

alanchiao commented Jun 16, 2020

@SaoirseARM: actually taking a closer look, this isn't unit tested. Can you add unit testing?

@Ruomei also mentioned that testing with this PR enables conversion of a clustered model to TFLite via from_keras_model in TF2, which should also be unit tested so that it continues to work.

@SaoirseARM SaoirseARM force-pushed the toupstream/cluster_saved_model branch from c22f134 to 3c951d9 Compare June 24, 2020 13:49
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no PR contributor has not signed CLA and removed cla: yes PR contributor has signed CLA labels Jun 24, 2020
@SaoirseARM SaoirseARM force-pushed the toupstream/cluster_saved_model branch from e379bb8 to 1e15af0 Compare June 24, 2020 14:28
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes PR contributor has signed CLA and removed cla: no PR contributor has not signed CLA labels Jun 24, 2020
@SaoirseARM
Copy link
Contributor Author

Hi @alanchiao , I have added two tests. One integration test including conversion to tflite and one unit test to ensure the stripped kernel is correct.


self._verify_tflite(tflite_file, self.x_train)

os.remove(keras_file)
Copy link

@alanchiao alanchiao Jun 24, 2020

Choose a reason for hiding this comment

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

Feel free to cleanup in a followup PR: os.remove should not be necessary since tempfile will automatically delete it.

@alanchiao alanchiao added ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. and removed ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. labels Jun 24, 2020
@alanchiao
Copy link

Thanks @SaoirseARM. Left a comment for the future.

Merging this PR for now since other work is depending on it.

@alanchiao
Copy link

@SaoirseARM: actually last thing. Could you squash all your commits into one so that there is a clean commit history? Doing the cleanup should generally be done prior to merging a PR.

@SaoirseARM SaoirseARM force-pushed the toupstream/cluster_saved_model branch from 1e15af0 to 66e206d Compare June 24, 2020 15:56
@alanchiao alanchiao added ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. and removed ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. labels Jun 24, 2020
@SaoirseARM
Copy link
Contributor Author

Thanks @alanchiao , I have squashed commits now down to one. Please let me know if I should update anything else.

@copybara-service copybara-service bot merged commit 888621b into tensorflow:master Jun 24, 2020
@SaoirseARM SaoirseARM deleted the toupstream/cluster_saved_model branch June 25, 2020 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes PR contributor has signed CLA ready to pull Working to get PR submitted to internal repository, after which merging to Github happens.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants