Skip to content

Make entrypoints inside Microsoft.ML.Transforms project internal #2288

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
shmoradims opened this issue Jan 28, 2019 · 5 comments
Closed

Make entrypoints inside Microsoft.ML.Transforms project internal #2288

shmoradims opened this issue Jan 28, 2019 · 5 comments
Labels
API Issues pertaining the friendly API
Milestone

Comments

@shmoradims
Copy link

We have 20+ entrypoints classes inside Microsoft.ML.Transforms like this one. I think they should be made internal because the end-user would use the MLContext extensions instead, not these entrypoints. Also, the entrypoints documentation doesn't get rendered properly because the TlcModule.EntryPoint annotation is not understood by docs.microsoft.com (example: #1725)

Thoughts?

/cc @TomFinley @sfilipi @eerhardt

@eerhardt
Copy link
Member

I agree that EntryPoints aren't intended to be our "public API", just yet - if ever.

@TomFinley logged a related issue with #2279:

A different example of this would be things like Microsoft.ML.EntryPoints, which we have to make public for "reasons," but cannot reasonably be said to have a public API.

@TomFinley TomFinley added the API Issues pertaining the friendly API label Jan 29, 2019
@TomFinley
Copy link
Contributor

Thank you for taking the initiative on this @shmoradims !

This I agree with... indeed I just stumbled upon this issue, I do much of the hiding of entrypoints in #2300. (Where I also make it so that non-public methods can still be visible in the entry-point catalog, since that was not the case previously. Specifically, the commit in that PR labeled, "Internalize common outputs of entry-points and deal with resulting fallout.")

But I surely did not get everything, since I was not trying to solve this issue broadly, but merely those entry-points that publish very specific types that we do not want part of our public API surface. A more thorough review where we review every method decorated with TlcModule.EntryPoint, and make sure that it is in an internal class, or else if the class must be public for other reasons (e.g., some trainers publish their entry-points as a static method on the class itself, which is fine) then the method itself should be internal.

Nonetheless, possibly my approach in #2300 may prove useful as a guide for how someone could hypothetically continue that work, since I agree we must do it in a more comprehensive way. Probably after my PR goes in though, since otherwise we'd be stepping on each other's toes. 😄

I will make this part of Project 13, since it deals with getting the public API in good shape.

@shmoradims
Copy link
Author

Thanks @TomFinley @eerhardt for you input. I'll have a PR for it after Tom merges #2300.

@TomFinley
Copy link
Contributor

@shmoradims it is now merged, thank you.

@shmoradims
Copy link
Author

It turned out that #2300 already covers everything in my PR, so I don't have to do anything :)

@shauheen shauheen added this to the 0119 milestone Jan 31, 2019
@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

4 participants