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

Change the TrainableModel::summary API to return ModelSummary #135

Closed
avan1235 opened this issue Jun 16, 2021 · 8 comments
Closed

Change the TrainableModel::summary API to return ModelSummary #135

avan1235 opened this issue Jun 16, 2021 · 8 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@avan1235
Copy link
Contributor

In current implementation the TrainableModel::summary return the list with the descriptions for the following layers.

This information is readable but doesn't allow for easy transformation of model summary gathered data as it is give as raw String. Additionally, the implementation classes log the provided layers description in more readable way, but it is only printed on stdio and not available at least as pretty formatted string that could bu printed later. The problem is more annoying in jupyter notebooks where we don't have an access to stdio and so the logged pretty summary is not visible.

I would propose creating some simple ModelSummary data class that would be responsible for holding the model summary data and would have some good implementation of toString() method that could be easily printed. It would allow for creating custom printing methods for ModelSummary as well as good keeping of model summary data as a single object instead of List<String>

@knok16
Copy link
Contributor

knok16 commented Aug 25, 2021

What do you think about the next changes?

  1. Classes to represent a model summary
data class ModelSummary(
    val type: String,
    val name: String,
    val layersSummaries: List<LayerSummary>,
    val trainableParamsCount: Long,
    val frozenParamsCount: Long,
    val totalParamsCount: Long
)

data class LayerSummary(
    val name: String,
    val type: String,
    val shape: Shape,
    val paramsCount: Long,
    val inputLayers: List<String>
)
  1. Make TrainableModel::summary return ModelSummary
  2. Remove all the logging from TrainableModel::summary so it will not have any side effects
  3. Provide some functions that will print ModelSummary to console/logger

@zaleslaw
Copy link
Collaborator

Hi, @knok16. Give me a few days to think about it; I'll return to you in a couple of days.

@zaleslaw zaleslaw self-assigned this Aug 31, 2021
@zaleslaw zaleslaw added this to the 0.4 milestone Sep 1, 2021
@zaleslaw zaleslaw added the enhancement New feature or request label Sep 1, 2021
@zaleslaw
Copy link
Collaborator

zaleslaw commented Sep 7, 2021

Thanks for lighting the problem @avan1235. I totally agree about that the existing solution is not good for notebooks.
So great to have a few data classes like @knok16 proposed.

I suppose the proposed solution should contain the following things:

  • data classes for Model and layer summary
  • printSummary and logSummary functions (logging could print on a console too)
  • tests for Sequential, Functional models and a few popular layers like Conv2D, ZeroPadding2D
  • refactoring for all examples which contain it.summary() to it.logSummary() for consistency.
  • 1 executed notebook with the built artifact from the proposed PR to estimate how pretty it is

@knok16
Copy link
Contributor

knok16 commented Sep 7, 2021

Can I take it?

@zaleslaw zaleslaw assigned knok16 and unassigned zaleslaw Sep 7, 2021
@zaleslaw
Copy link
Collaborator

zaleslaw commented Sep 7, 2021

@knok16 Sure, it's yours

@knok16
Copy link
Contributor

knok16 commented Sep 7, 2021

changes for pp 1-3 #216
will do p.4 in separate PR, to not clutter this one

@zaleslaw
Copy link
Collaborator

Mostly closed in the #216
I will release an artifact, load it to the Maven Central and make a double check on the Datalore and Jupyter notebooks

@zaleslaw zaleslaw modified the milestones: 0.4, 0.3 Sep 17, 2021
@zaleslaw
Copy link
Collaborator

I've checked the 0.3.0-alpha-4 artifact, print summary works as expected and this issue has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants