Skip to content

adding a dependency to the MlNetMklDeps package #594

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 1 commit into from
Jul 30, 2018

Conversation

sfilipi
Copy link
Member

@sfilipi sfilipi commented Jul 28, 2018

Introducing the entry point for Ols, and enabling its test
Adding documentation for Ols

Resolves #497

Introducing the entry point for Ols, and enabling its test
Adding documentation  for Ols
@@ -11,6 +11,7 @@
<ProjectReference Include="..\Microsoft.ML.CpuMath\Microsoft.ML.CpuMath.csproj" />
<ProjectReference Include="..\Microsoft.ML.Data\Microsoft.ML.Data.csproj" />
<ProjectReference Include="..\Microsoft.ML\Microsoft.ML.csproj" />
<PackageReference Include="MlNetMklDeps" Version="$(MlNetMklDepsPackageVersion)" />
</ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. I wonder if OLS really belongs in standard learners if it has native dependencies. Well, we'll possibly resolve that later.

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Thanks for handling this sticky situation @sfilipi .

@TomFinley
Copy link
Contributor

TomFinley commented Jul 28, 2018

Just wondering @sfilipi , what is the relationship of this with #578 ? I assume this PR supersedes it, and you found some other way to publish the nuget? Or does it depend on #578, and we will be consuming a nuget that is built out of the same repo? (Though presumably via some other process.) #Pending

@sfilipi
Copy link
Member Author

sfilipi commented Jul 29, 2018

Thanks @TomFinley for reviewing.
I added WIP to one of the commit comments of #578, and github keeps the WIP tag, if it sees WIP in the commit comments, or PR title.
Rather than fiddling with rebasing and amending commit, created a new PR. I'll close the other one.. #Resolved

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.

thanks for doing this.!

@sfilipi sfilipi merged commit dcc8ae8 into dotnet:master Jul 30, 2018
@eerhardt
Copy link
Member

    [Fact(Skip = "Need CoreTLC specific baseline update")]

This test is still skipped. Can we enable it?


Refers to: test/Microsoft.ML.Predictor.Tests/TestPredictors.cs:891 in 0f37217. [](commit_id = 0f37217, deletion_comment = False)

@eerhardt
Copy link
Member

    [Fact(Skip = "Need CoreTLC specific baseline update")]

Same here - can this test be enabled?


Refers to: test/Microsoft.ML.Predictor.Tests/TestPredictors.cs:902 in 0f37217. [](commit_id = 0f37217, deletion_comment = False)

@@ -11,6 +11,7 @@
<ProjectReference Include="..\Microsoft.ML.CpuMath\Microsoft.ML.CpuMath.csproj" />
<ProjectReference Include="..\Microsoft.ML.Data\Microsoft.ML.Data.csproj" />
<ProjectReference Include="..\Microsoft.ML\Microsoft.ML.csproj" />
<PackageReference Include="MlNetMklDeps" Version="$(MlNetMklDepsPackageVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

This isn't going to work because the Microsoft.ML package doesn't reference the MlNetMklDeps package (and in my opinion, it should never reference it).

We will need to split OLS (and any other functionaltiy that depends on MKL) out into a separate NuGet package.

In general, if a component takes a dependency on "non-standard" or "large" external dependencies, we can't ship that component in our "Main" nuget package.

codemzs pushed a commit to codemzs/machinelearning that referenced this pull request Aug 1, 2018
Introducing the entry point for Ols, and enabling its test
Adding documentation  for Ols
@sfilipi sfilipi deleted the mkldepsfinal branch October 26, 2018 18:49
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants