-
Notifications
You must be signed in to change notification settings - Fork 336
[Clustering] Support for clustering of a subclassed model. #571
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
Conversation
Change-Id: I85156df4d29750a11c8d683844104d8a3d61204a
| self.assertLessEqual(nr_unique_weights, self.params["number_of_clusters"]) | ||
|
|
||
| @keras_parameterized.run_all_keras_modes(always_skip_v1=True) | ||
| def testEndToEndSubclassedModelTwoLayers(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test re-produces the approach tested here: #554
| @keras_parameterized.run_all_keras_modes(always_skip_v1=True) | ||
| def testEndToEndSubclassedModelAsDeepLayer(self): | ||
| """Test End to End clustering for the model with the layer as a subclass model.""" | ||
| # This case is not supported currently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this case will be enabled later once the current approach is approved
| json.loads(clustered_model.to_json())) | ||
| self.assertEqual(loaded_model.built, True) | ||
|
|
||
| @keras_parameterized.run_all_keras_modes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks that the block of these tests has not been upstreamed. i can remove it from my PR as they are not relevant
|
Hi @alanchiao Please take a look at this PR: it enables clustering for a subclassed model. |
|
Haven't looked into this PR in detail yet, but the main thing is that we need to see how well this approach actually works, from the doc I had shared. The comments on the side of the doc need to be resolved in a satisfactory manner. Ultimately, when any person asks for subclassed model support, they're really looking for a task or model to be supported (Object Detection, NLP) and until we are confident it works reasonably with at least one of those using a standard implementation (e.g. TF Official Models), then it doesn't add significant value. |
|
Hi @alanchiao Yes, all issues that are mentioned in the doc are addressed. The clustering case is easier, because we cluster the trained model always. The example on mnist demonstrates that we use the same Clustering API - I just removed summary() and save/load() into HDF5 as they are not supported by the subclassed model. |
|
@wwwind : it's not immediately obvious why the fact that clustering is always applied to a pre-trained model makes a difference. The suggested problem comes from setting the attributes of the layers of a subclassed model to the optimized versions of those layers, which seems independent of the model optimization technique. That said, I haven't fully understood the risks / issues yet (e.g. I questioned further on the second issue for instance). If you and others feel comfortable with moving forward with it though, you can choose to do so. It may be good to say that the approach hasn't been tested on more complex cases such as Object Detection in Official Models. |
… of tf greater than 2.4.0. This change is to fix this. Change-Id: Ic794c5945f9acf4e12f92c5bd803f13fde7094bd
|
it is not relevant anymore |
This PR adds support for clustering of a subclassed model. Added an example for clustering of a subclassed model taken from the tutorial: https://www.tensorflow.org/tutorials/quickstart/advanced