Skip to content

Hide much of Microsoft.ML.Model namespace. #2649

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Feb 20, 2019
Merged

Hide much of Microsoft.ML.Model namespace. #2649

merged 6 commits into from
Feb 20, 2019

Conversation

TomFinley
Copy link
Contributor

@TomFinley TomFinley commented Feb 20, 2019 •
edited
Loading

Fixes #2640. Per usual commits are logically ordered to reflect the natural order in which structures are internalized, etc.

@TomFinley TomFinley added the API Issues pertaining the friendly API label Feb 20, 2019
@TomFinley TomFinley self-assigned this Feb 20, 2019
@codecov
Copy link

codecov bot commented Feb 20, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@bd00c1e). Click here to learn what that means.
The diff coverage is 80%.

@@            Coverage Diff            @@
##             master    #2649   +/-   ##
=========================================
  Coverage          ?   71.54%           
=========================================
  Files             ?      801           
  Lines             ?   141840           
  Branches          ?    16119           
=========================================
  Hits              ?   101474           
  Misses            ?    35916           
  Partials          ?     4450
Flag Coverage Δ
#Debug 71.54% <80%> (?)
#production 67.83% <80%> (?)
#test 85.74% <ø> (?)
Impacted Files Coverage Δ
...L.Data/Evaluators/MultiClassClassifierEvaluator.cs 77.69% <ø> (ø)
...StandardLearners/Standard/LinearModelParameters.cs 60.63% <ø> (ø)
.../Microsoft.ML.Data/Prediction/CalibratorCatalog.cs 90.71% <ø> (ø)
src/Microsoft.ML.Core/Data/ModelLoadContext.cs 84.12% <ø> (ø)
src/Microsoft.ML.Core/Data/ModelHeader.cs 83.8% <ø> (ø)
src/Microsoft.ML.Core/Data/ModelSaving.cs 93.33% <ø> (ø)
src/Microsoft.ML.Core/Data/Repository.cs 75.78% <ø> (ø)
...Microsoft.ML.Data/DataLoadSave/TransformerChain.cs 87.03% <ø> (ø)
src/Microsoft.ML.Core/Data/ModelLoading.cs 74.45% <ø> (ø)
...rc/Microsoft.ML.Data/Transforms/InvertHashUtils.cs 96.06% <ø> (ø)
... and 5 more

@codemzs
Copy link
Member

codemzs commented Feb 20, 2019 

    public const string NormalizerWarningFormat =

This can be made internal. It is just used at WarnOnOldNormalizer which is a private method.

Refers to: src/Microsoft.ML.Data/Dirty/ModelParametersBase.cs:17 in 5796f03. [](commit_id = 5796f03, deletion_comment = False)

@TomFinley
Copy link
Contributor Author 

    public const string NormalizerWarningFormat =

Ah, yes, thanks, missed this one somehow.

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

Refers to: src/Microsoft.ML.Data/Dirty/ModelParametersBase.cs:17 in 5796f03. [](commit_id = 5796f03, deletion_comment = False)

@codemzs
Copy link
Member

codemzs commented Feb 20, 2019 

    public const string RegistrationName = "LaplacianRandom";

internal/private protected?

Refers to: src/Microsoft.ML.Transforms/FourierDistributionSampler.cs:148 in 5796f03. [](commit_id = 5796f03, deletion_comment = False)

codemzs
codemzs approved these changes Feb 20, 2019
View reviewed changes
Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

:shipit:

@codemzs
Copy link
Member

codemzs commented Feb 20, 2019 

    public const string LoaderSignature = "RandLaplacianFourierExec";

internal/private protected.

Refers to: src/Microsoft.ML.Transforms/FourierDistributionSampler.cs:147 in 5796f03. [](commit_id = 5796f03, deletion_comment = False)

@TomFinley
Copy link
Contributor Author

TomFinley commented Feb 20, 2019 •
edited
Loading 

    public const string LoaderSignature = "RandLaplacianFourierExec";

internal/private protected.

Refers to: src/Microsoft.ML.Transforms/FourierDistributionSampler.cs:147 in 5796f03. [](commit_id = 5796f03, deletion_comment = False)

You're not wrong in the sense that this file should be addressed, but I might prefer to address this and similar matter in work more appropriate to that task.

If it was just one line I'd probably opportunistically fix it, but this file needs some work. Seems related to #1973.

Edit: Looks like this was mostly already the subject of a PR by @artidoro !

@TomFinley TomFinley requested a review from artidoro February 20, 2019 21:49
@artidoro
Copy link
Contributor

artidoro commented Feb 20, 2019 •
edited
Loading 

    {

Should this be internal? #Closed

Refers to: src/Microsoft.ML.Data/DataLoadSave/TransformerChain.cs:203 in 888be1f. [](commit_id = 888be1f, deletion_comment = False)

artidoro
artidoro reviewed Feb 20, 2019
View reviewed changes
src/Microsoft.ML.Data/DataLoadSave/TransformerChain.cs
@@ -239,7 +239,7 @@ public static class TransformerChain
{
public const string LoaderSignature = "TransformerChain";

Copy link
Contributor

@artidoro artidoro Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think this can be made internal. We usually do that with the LoaderSignature elsewhere. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No doubt. However I wasn't trying to do that.

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

artidoro
artidoro reviewed Feb 20, 2019
View reviewed changes
src/Microsoft.ML.Data/DataLoadSave/TransformerChain.cs
@@ -239,7 +239,7 @@ public static class TransformerChain
{
Copy link
Contributor

@artidoro artidoro Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Now that I think about it, I suspect that this entire class can be made internal. We should expose these methods in MLContext if we need them, and they are not there yet. Please let me know if I am wrong. #Closed

Copy link
Contributor Author

@TomFinley TomFinley Feb 20, 2019 •
edited by artidoro
Loading

Choose a reason for hiding this comment

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

That's all fine, but, maybe we could internalize things unrelated to the issue and this PR in subsequent PRs? My goal @artidoro is not to internalize the entire assembly in one go, just some specific types and infrastructure I see in the Microsoft.ML.Model namespace, as described in the issue, title of the PR, and so on. Perhaps we could evaluate the worth of the PR along those dimensions? #Resolved

Copy link
Contributor Author

@TomFinley TomFinley Feb 20, 2019 •
edited by artidoro
Loading

Choose a reason for hiding this comment

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

So to be clear, I don't really intend to do this. Unless I literally can't get a signoff any other way. #Resolved

@TomFinley
Copy link
Contributor Author 

    {

Maybe. This however is not the subject of this PR. Just to remind you, the title of PR is "Hide much of Microsoft.ML.Model namespace", which does not include this.

I anticipate that in other PRs as we are finalizing parts of our API surface, you will find people touching files that themselves require internalization work.

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

Refers to: src/Microsoft.ML.Data/DataLoadSave/TransformerChain.cs:203 in 888be1f. [](commit_id = 888be1f, deletion_comment = False)

artidoro
artidoro approved these changes Feb 20, 2019
View reviewed changes
Copy link
Contributor

@artidoro artidoro left a comment

Choose a reason for hiding this comment

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

:shipit:

@TomFinley TomFinley merged commit a1b66ac into dotnet:master Feb 20, 2019
@TomFinley TomFinley deleted the HideModel branch February 20, 2019 23:15
@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.
Reviewers

@codemzs codemzs codemzs approved these changes

@artidoro artidoro artidoro approved these changes

@Ivanidzo4ka Ivanidzo4ka Awaiting requested review from Ivanidzo4ka

@wschin wschin Awaiting requested review from wschin

@yaeldekel yaeldekel Awaiting requested review from yaeldekel

@najeeb-kazmi najeeb-kazmi Awaiting requested review from najeeb-kazmi

@sfilipi sfilipi Awaiting requested review from sfilipi

Assignees

@TomFinley TomFinley

Labels
API Issues pertaining the friendly API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TomFinley @codemzs @artidoro