Skip to content
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

Farewell to the Static API #4009

Merged
merged 10 commits into from
Jul 30, 2019
Merged

Farewell to the Static API #4009

merged 10 commits into from
Jul 30, 2019

Conversation

artidoro
Copy link
Contributor

Fixes #3952.

In this PR I 1. remove the static API code and 2. migrate the tests to the C# API (dynamic API) and 3. stop producing the static pipe nugets.

The commits are organized logically, which should simplify the review process.

@artidoro artidoro self-assigned this Jul 16, 2019
@codemzs codemzs changed the title Staticapiremove Farewell to the Static API Jul 16, 2019
@codemzs
Copy link
Member

codemzs commented Jul 16, 2019

Were you thinking of removing references to dynamic API? and just renaming it to API?

@codemzs
Copy link
Member

codemzs commented Jul 16, 2019

// The .NET Foundation licenses this file to you under the MIT license.

why is this file showing as modified? I don't see any changes. #Resolved


Refers to: test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs:2 in ee68b9c. [](commit_id = ee68b9c, deletion_comment = False)

@artidoro
Copy link
Contributor Author

// The .NET Foundation licenses this file to you under the MIT license.

Good point, I'll remove it from the PR.


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


Refers to: test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs:2 in ee68b9c. [](commit_id = ee68b9c, deletion_comment = False)

@artidoro
Copy link
Contributor Author

I can do it in this PR or in another PR. It might be a more documentation focused PR, so I was not thinking to have in this PR initially. What do you think?


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

@eerhardt
Copy link
Member

eerhardt commented Jul 17, 2019

namespace FakeStaticPipes

Shouldn't this file be deleted? #Resolved


Refers to: test/Microsoft.ML.StaticPipelineTesting/StaticPipeFakes.cs:16 in ee68b9c. [](commit_id = ee68b9c, deletion_comment = False)

@artidoro
Copy link
Contributor Author

namespace FakeStaticPipes

Yes you are right, I had removed it from the solution but not the files from the repo.


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


Refers to: test/Microsoft.ML.StaticPipelineTesting/StaticPipeFakes.cs:16 in ee68b9c. [](commit_id = ee68b9c, deletion_comment = False)

@codemzs codemzs requested a review from natke July 26, 2019 17:04
@@ -73,11 +72,6 @@
</None>
<PackageReference Include="Microsoft.ML.TensorFlow.Redist" Version="0.10.0" />

<ProjectReference Include="..\..\..\src\Microsoft.ML.Analyzer\Microsoft.ML.Analyzer.csproj">
Copy link
Member

@eerhardt eerhardt Jul 30, 2019

Choose a reason for hiding this comment

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

Why this change?

Nevermind, I see that we are removing the analyzer because it doesn't analyze anything other than static API usage.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@artidoro artidoro merged commit 59699a5 into dotnet:master Jul 30, 2019
@artidoro
Copy link
Contributor Author

Thank you for reviewing!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 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.

Remove Static API code
4 participants