Fix to add clustering_callbacks.py to the pip package

[benkli01]

This is a small fix to add a dependency on clustering_callbacks.py so that the file is added to the pip package.

[akarmi]

Looks OK to me, but @alanchiao, please have a look if this is a correct way of instructing bazel to include this module into the wheel.

[alanchiao]

@benkli01

Please refer to an example like #421 for adding changes to the pip package. You can subsequently install from source and test the actual path like in https://www.tensorflow.org/model_optimization/guide/install#installing_from_source.

[alanchiao]

Please also refer to https://github.com/tensorflow/model-optimization/blob/master/CONTRIBUTING.md#style-and-practices in updating the release notes given this feature.

[benkli01]

Hi @alanchiao , thanks for the feedback.

• Regarding the release notes: Is the RELEASE.md in branch master ready to be edited? Should I just overwrite the template or add a new section for the next release?
• I have added the ClusteringSummaries to the clustering API as well (not only to the BUILD file).

[alanchiao]

Yes the release notes are ready to be edited. You can add a new section for the next release with a TBD release version.

[alanchiao]

Changes otherwise look good.

To now actually do the API review since the previous PR didn't expose anything: it'd be good to:

a) Include a screenshot of a sample of what you'd see on Tensorboard from these summaries
b) Just as a note for others: ClusteringSummaries sets a default of 'logs' for log_dir, which matches the TensorBoard callback but differs from PruningSummaries. Really, it makes sense to do what was
done for ClusteringSummaries.
c) Is there a reason to have a separate cluster_update_freqfrom update_freq? Really, it's probably best of PruningSummaries didn't inherent from the TensorBoard callback - it's confusing since if a user uses both PruningSummaries and TensorBoard callbacks, then the TensorBoard callback work would be done twice.

[alanchiao]

We need to otherwise get a look by either @nutsiepully or @liyunlu0618.

[alanchiao]

@benkli01: could you comment on "is there a reason to have a separate cluster_update_freq from update_freq? Really, it's probably best of PruningSummaries didn't inherent from the TensorBoard callback - it's confusing since if a user uses both PruningSummaries and TensorBoard callbacks, then the TensorBoard callback work would be done twice". It's the main thing that stood out to me in this API.

[benkli01]

@benkli01: could you comment on "is there a reason to have a separate cluster_update_freq from update_freq? Really, it's probably best of PruningSummaries didn't inherent from the TensorBoard callback - it's confusing since if a user uses both PruningSummaries and TensorBoard callbacks, then the TensorBoard callback work would be done twice". It's the main thing that stood out to me in this API.

Hi @alanchiao , I am really sorry for the delay. There were some internal discussions about this PR.
The callback is derived from the TensorBoard callback because it produces very consistent summaries and the PruningSummaries callback does it as well, so I thought it would be best to be consistent, but I understand your point. From the user perspective adding another callback might be more intuitive than replacing the TensorBoard callback. It might lead to problems/inconsistencies when writing the summaries to the disk though, but I would have to further investigate that.

[alanchiao]

Thanks Benjamin for the response. I think it's fine to be consistent with inheriting from the callback for now given it's what Pruning does.

I actually intended to highlight the part ""is there a reason to have a separate cluster_update_freq from update_freq" - is it valuable to have the two separate?
I accidentally copied more of the quote than I should have.

[benkli01]

I actually intended to highlight the part ""is there a reason to have a separate cluster_update_freq from update_freq" - is it valuable to have the two separate?

This is actually the main addition of the ClusteringSummaries. It allows to save histograms of the clustered weights per batch instead of just per epoch, so that the clustering can be observed at a finer scale. To be honest, after thinking about it I'm not sure if that is enough to justify the new class.

[daverim]

LGTM