Skip to content

Consider removing Sweeper, PipelineInference, and ResultProcessor assemblies from Microsoft.ML nuget #689

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
eerhardt opened this issue Aug 17, 2018 · 14 comments · Fixed by #2690
Assignees
Labels
API Issues pertaining the friendly API
Milestone

Comments

@eerhardt
Copy link
Member

We currently have a few assemblies in our Microsoft.ML nuget package that aren't part of what we want in our public API surface area.

These assemblies include:

  • Microsoft.ML.Maml
  • Microsoft.ML.ResultProcessor
  • Microsoft.ML.PipelineInference
  • Microsoft.ML.Sweeper

The last two may some day be part of our public API, but in their current form we don't want to expose them as public API.

We should remove these assemblies from our nuget package (maybe put them in a separate nuget package), so external users don't depend on these types. That way we are free to update the API until we are ready to ship it as an official API.

/cc @Zruty0 @TomFinley

@TomFinley
Copy link
Contributor

TomFinley commented Aug 20, 2018

Hi @eerhardt thanks for looking at this. I am certain these do not belong in Microsoft.ML package at all, but what I'm less certain about is whether we need to publish any of them in a nuget.

I am fairly confident that the Maml package exists for test support and the console project, and does not need to be in any nuget I believe. But I'd gladly learn better.

Regarding pipeline inference and sweeping, and maybe result processor, @sfilipi might know best about what the current dependency structure is w.r.t. GUI. Some of this code might need to be in a nuget for the sake of that, assuming that the way that consumes these packages is via nuget. But I'm not sure if the GUI actually depends on any of that. @justinormont is I think possibly consuming some of these packages, but how he is doing it (whether by nuget or some other mechanism), I do not know.

Edit: Given that usage, if any, would be for in progress work that would probably be working off master branch anyway, one practical solution would be to remove them from the nuget, see who complains (if anyone), then fix if anyone complains.

@eerhardt
Copy link
Member Author

@justinormont - @TomFinley and I are considering removing the PipelineInference code from dotnet/machinelearning all together. The reasoning is that we don't expect to ship this assembly any time soon, and it is impeding our progress making the new API changes. We currently need to keep it building, running, etc.

If we remove it from the repo, we will have it in the source control history, and can always bring it back when we are ready to ship it.

Thoughts?

@justinormont
Copy link
Contributor

justinormont commented Aug 24, 2018

@eerhardt : Can you ping Nader and @abgoswam? I will be out for a week (until ~Sept 4rd).

Removing the PipelineInference code from the codebase would be rather detrimental. One option would be to move it towards the internal repo. In the end, this is required by our GUIs and AutoML system (and another two <redacted> projects).

cc: @markusweimer, who may want to chime in regard to the AutoML project's ongoing status.

@justinormont
Copy link
Contributor

I'd recommend discussing w/ @RagingBull630 (Nader Albussam).

@TomFinley
Copy link
Contributor

Hi @justinormont and @abgoswam , perhaps one of you or someone else could, if you cared, move it out? We might remove it soon, since we're in the process of hiding anything we don't want in the public surface area, especially more experimental work like this.

If we remove it before you get to it, you can of course simply fetch it again from the history to "resurrect" it in a place of your choosing.

@justinormont
Copy link
Contributor

PipelineInference is under active development. Last I talked w/ @chris-lauren we are expecting to move forward and bring this more in to the forefront.

@TomFinley
Copy link
Contributor

TomFinley commented Nov 6, 2018

That's fine, but if under active development, it seems like that development is not happening in ML.NET anyway -- the changelogs for the associated directories has not seen much progress. It is not part of our public API. Nor would it ever really need to since pipeline sweeping is something that can use ML.NET but doesn't have to be ML.NET, right?

At least searching open or closed issues for PipelineInference reveals nothing about future development. Outside of github, I have seen no mention of it in API reviews (beyond of course the desire to remove it from our repo), nor in the active project lists do I see mention of it. It is not clear to me it is under active development in any meaningful sense of the word.

Feel free to share specific reasons for your request to keep it.

@ganik
Copy link
Member

ganik commented Dec 22, 2018

@TomFinley @eerhardt
NimbusML references PipelineInference, and the build is currently breaking:
The type or namespace name 'PipelineInference' does not exist in the namespace 'Microsoft.ML.Runtime' (are you missing an assembly reference?)

Is there anyway I can fix this in NimbusML code?

@eerhardt
Copy link
Member Author

eerhardt commented Jan 2, 2019

It looks like the whole Microsoft.ML.PipelineInference library was removed with 37176a1.

What specifically does NimbusML use PipelineInference for?

@montebhoover
Copy link
Contributor

Attempting to remove references to PipelineInference and AutoInference...

@eerhardt eerhardt added the API Issues pertaining the friendly API label Feb 14, 2019
@Ivanidzo4ka
Copy link
Contributor

@ganik do you know what is current status of PipelineInference in NimbusML?
Can we safely remove this libraries from ML package?

@eerhardt
Copy link
Member Author

NimbusML (or any other consumer) can still use these libraries if we put them in their own NuGet packages. The important piece for this work is that the above assemblies should not be in the Microsoft.ML NuGet package. That package is only for "core" libraries that everyone needs.

@Ivanidzo4ka
Copy link
Contributor

Ok, let's move them to separate Nuget packages (only Sweeper and PipelineInference) and delete 4 projects from Microsoft.ML
@singlis can you take care of this?

@singlis
Copy link
Member

singlis commented Feb 20, 2019

Yes - Ill take a look, thank you.

singlis added a commit to singlis/machinelearning that referenced this issue Feb 22, 2019
- Removed ResultProcessor and Maml from Microsoft.ML nuget. These are
currently not in a nuget package
- Moved Sweeper from Microsoft.ML to Microsoft.ML.Sweep.
- Updated nuget references as there were some other non-related errors
for ONNX Transformer.

This fixes dotnet#689
singlis added a commit to singlis/machinelearning that referenced this issue Feb 23, 2019
singlis added a commit to singlis/machinelearning that referenced this issue Feb 23, 2019
Found while fixing dotnet#689, moved to separate commit.
eerhardt pushed a commit that referenced this issue Feb 23, 2019
Found while fixing #689, moved to separate commit.
@shauheen shauheen added this to the 0219 milestone Feb 25, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 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 a pull request may close this issue.

8 participants