Skip to content

ITransformer derives from ICanSaveModel and explicit implementation for ICanSaveModel #2431

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

Merged
merged 10 commits into from
Feb 9, 2019

Conversation

artidoro
Copy link
Contributor

@artidoro artidoro commented Feb 6, 2019

Fixes #2336.

This PR makes ITransformer derive from ICanSaveModel. The reasons for changing the core ITransformer/IEstimator/IDataView API are illustrated in more detail in the issue #2336. But this essentially captures the idea that once trained the model should be savable.
For this change I moved ICanSaveModel, ModelSaveContext, ModelLoadContext and a few related utility files to Microsoft.ML.Core as ITransformer lives in that assembly.

The second dimension of this change is the explicit implementation of the interface ICanSaveModel everywhere. This will make it a requirement to cast the classes to ICanSaveModel in order to access the method Save(ModelSaveContext ctx) so that it is less visible. I have covered the entire code base with my changes, including some classes that are internal and others that I think should be made internal shortly. This makes my change touch ~140 files. I decided to do so in order to make the way we implement the ICanSaveModel interface consistent everywhere.

@artidoro artidoro self-assigned this Feb 6, 2019
@artidoro artidoro added the API Issues pertaining the friendly API label Feb 6, 2019
@artidoro artidoro force-pushed the savemodel branch 2 times, most recently from 1850a8a to 5fd88bf Compare February 6, 2019 09:08
@TomFinley
Copy link
Contributor

Looks mostly good @artidoro except for one or two unnecessary internalizations. Do we have though a handle on why these ReturnTypeIsSchemaShape and ReturnTypeIsSchemaShapeChained tests are failing? Are you planning on de-WIPing this?

@artidoro
Copy link
Contributor Author

artidoro commented Feb 7, 2019

Thank you very much for taking a look Tom! I am resolving your comments, and de-WIPing. Will look into the test failures afterwards.


In reply to: 461297633 [](ancestors = 461297633)

@artidoro artidoro changed the title WIP: ITransformer derives from ICanSaveModel and explicit implementation for ICanSaveModel ITransformer derives from ICanSaveModel and explicit implementation for ICanSaveModel Feb 7, 2019
@codecov
Copy link

codecov bot commented Feb 8, 2019

Codecov Report

Merging #2431 into master will decrease coverage by 0.01%.
The diff coverage is 75.43%.

@@            Coverage Diff             @@
##           master    #2431      +/-   ##
==========================================
- Coverage   71.23%   71.21%   -0.02%     
==========================================
  Files         786      786              
  Lines      140989   141003      +14     
  Branches    16114    16114              
==========================================
- Hits       100427   100416      -11     
- Misses      36091    36118      +27     
+ Partials     4471     4469       -2
Flag Coverage Δ
#Debug 71.21% <75.43%> (-0.02%) ⬇️
#production 67.57% <75%> (-0.02%) ⬇️
#test 85.28% <100%> (ø) ⬆️

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Feb 8, 2019

public sealed class ProduceIdTransform : RowToRowTransformBase

if you have intent of pushing another iteration, you can make this transform internal :) #Resolved


Refers to: src/Microsoft.ML.Transforms/ProduceIdTransform.cs:30 in 5b44cae. [](commit_id = 5b44cae, deletion_comment = False)

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@artidoro artidoro merged commit 6526a01 into dotnet:master Feb 9, 2019
@artidoro artidoro deleted the savemodel branch March 13, 2019 17:55
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 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

Successfully merging this pull request may close these issues.

4 participants