Skip to content

ITransformer should derive from ICanSaveModel and we should explicitly implement ICanSaveModel #2336

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

Closed
artidoro opened this issue Jan 30, 2019 · 1 comment
Assignees
Labels
API Issues pertaining the friendly API

Comments

@artidoro
Copy link
Contributor

artidoro commented Jan 30, 2019

In our code base we make the implicit assumption that all classes implementing ITransformer also implement ICanSaveModel. ITransformer is the abstraction that transforms the data and both single and chained components in a pipeline are made into an ITransformer once they are trained. Since this is the abstraction that contains trained models and data transformations, it should also be savable to a file. The standard scenario is that after training, one saves trained models, or ITransformer, for later use for scoring/transforming new data.

The above highlights that this is an important assumption and that we should capture it in code by making ITransformer derive from ICanSaveModel. Especially going forward as we think about allowing custom components, we should make it explicit that new transformers also implement ICanSaveModel.

Furthermore, we should explicitly implement in the transformers the Save method of the ICanSaveModel interface. This would make it less visible to the users. The reasons for doing so are twofold:

  1. ModelSaveContext only has internal constructors, so a user cannot instantiate it.
  2. We only use the Save method internally, and we don't want the user to call it directly.

/cc @TomFinley @sfilipi

@artidoro artidoro added the API Issues pertaining the friendly API label Jan 30, 2019
@artidoro artidoro self-assigned this Jan 30, 2019
@artidoro artidoro changed the title Save method in Transformers should be made internal ITransformer should derive from ICanSaveModel and we should explicitly implement ICanSaveModel Feb 5, 2019
@TomFinley
Copy link
Contributor

The above highlights that this is an important assumption and that we should capture it in code by making ITransformer derive from ICanSaveModel. Especially going forward as we think about allowing custom components, we should make it explicit that new transformers also implement ICanSaveModel.

Just like to double click on that to emphasize it. Due to ITransformer being an interface, it is essential that anything that is required for it be on it now. If, contrariwise, what we call ITransformer was instead an abstract class, we would have been free to not do this right now.

On the other, other hand, it is actually at this stage impossible for someone to implement ITransformer directly due to this issue, which might be sufficient protection, at least from a practical if not principled perspective. 😄 if ITransformer were also an interface, some future work we are also doing that requires multiple inheritance would be complicated (#2354).

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

No branches or pull requests

2 participants