Move DataFrame to machinelearning

[pgovind]

Move MDA, MDAI, MDA.Tests and MDAI.Tests to machinelearning.

For an easier time reviewing, the real changes are after the Merge branch 'port' of ../corefxlab into DataFrame_1 commit.

Just for posterity, I used git-filter-repo to pare down corefxlab locally to just the MDA and MDAI parts that I was interested in and then moved them over to the machinelearning repo. git-filter-repo was way faster than git filter-branch on my machine.

[ericstj]

We should try to decide how we want to describe package version over in the new repo.

[pgovind]

What do you mean? (I see that Versions.props is setting a default version of 1.5.5 for all the packages in the repo)

[eerhardt]

You can remove VersionPrefix here all together. And Microsoft.Data.Analaysis will just become an "unstable" project in machinelearning. Which means the next time we release it, it will be versioned 0.17.x.

See

machinelearning/eng/BranchInfo.props

Lines 3 to 23 in 8dd47c1

	<StableProjects>
	Microsoft.Extensions.ML;
	Microsoft.ML.DataView;
	Microsoft.ML.CpuMath;
	Microsoft.ML;
	Microsoft.ML.Core;
	Microsoft.ML.Data;
	Microsoft.ML.KMeansClustering;
	Microsoft.ML.PCA;
	Microsoft.ML.StandardTrainers;
	Microsoft.ML.Transforms;
	Microsoft.ML.FastTree;
	Microsoft.ML.ImageAnalytics;
	Microsoft.ML.LightGbm;
	Microsoft.ML.Mkl.Components;
	Microsoft.ML.Mkl.Redist;
	Microsoft.ML.TimeSeries;
	Microsoft.ML.TensorFlow;
	Microsoft.ML.OnnxTransformer;
	Microsoft.ML.Vision;
	</StableProjects>

machinelearning/eng/BranchInfo.props

Lines 35 to 39 in 8dd47c1

	<PropertyGroup Condition="'$(IsStableProject)' != 'true'">
	<MajorVersion>0</MajorVersion>
	<MinorVersion>17</MinorVersion>
	<PatchVersion>6</PatchVersion>
	</PropertyGroup>

[pgovind]

Hmmm, so 1.5.5 exists in both eng/Versions.props and eng/BranchInfo.props. Anyway, it's not important for our purposes here.

For now, using the version in BranchInfo.props is ok (we'd jump from 0.4.0 -> 0.17.6). But, if we wanted to release a new version of DataFrame, I don't think we'd want to increase the version for all the other projects right?

[eerhardt]

But, if we wanted to release a new version of DataFrame, I don't think we'd want to increase the version for all the other projects right?

My thoughts were that we would no longer "release a new version of DataFrame" by itself. It would now start shipping along with the rest of the ML.NET libraries on the same cycle. So when 0.17.7 ships next, we would ship Microsoft.Data.Analysis v0.17.7, same for 0.17.8, etc.

[eerhardt]

so 1.5.5 exists in both eng/Versions.props and eng/BranchInfo.props

It looks like arcade will overwrite VersionPrefix if MajorVersion and MinorVersion are set:

https://github.com/dotnet/arcade/blob/ca7fab569267ed3bc73360882d652d119aae5653/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L76

@michaelgsharp - I think we can delete the VersionPrefix line in eng/Versions.props.

[pgovind]

So when 0.17.7 ships next, we would ship Microsoft.Data.Analysis v0.17.7, same for 0.17.8, etc.

Perfect. I was thinking we should do a quick MDA 0.5.0 release this month and then get on to the ML releases, but it looks like we can release preview packages whenever we want in ML, so this is not a problem.

[michaelgsharp]

@eerhardt I'll run a test build and see. It would be much better to only have to update in 1 place. My local build is having issues though, so I'll test it when I am able to.

[ericstj]

This is pretty hacky. @Anipik has been looking at defining patterns for dotnet/runtime pkgproj -> csproj transition. I wonder if he has an idea for a better pattern for this case.

I see you're taking the one configuration of MS.D.A.Interactive, putting that assembly in interactive-extensions/dotnet and ignoring any of its dependencies. Do you have any more info to share about how the interactive-extensions/dotnet folder works? What is the expectation of components that use that convention?

[eerhardt]

Do you have any more info to share about how the interactive-extensions/dotnet folder works?

There isn't great documentation on this yet.

You can see some docs at https://github.com/dotnet/interactive/tree/main/docs#extending-net-interactive. Note there is an unlinked "Publishing your extension using NuGet" article that looks like it hasn't been written yet.

I think the best "doc" is the sample at:

https://github.com/dotnet/interactive/blob/main/samples/extensions/Library.nuget/Library.nuget.csproj

cc @jonsequitur @colombod @brettfo @LadyNaggaga

[ericstj]

What's the minimum TargetFramework required for dotnet interactive extensions? Are dependencies allowed? Should the extensions deps file be included?

I think the way this is included now would add the file multiple times if M.D.A had multiple TargetFrameworks. I think there might be a different hook other than TfmSpecificPackageFile to use for this scenario. I'll see if I can come up with a sample.

[eerhardt]

What's the minimum TargetFramework required for dotnet interactive extensions?

You need to reference: https://www.nuget.org/packages/Microsoft.DotNet.Interactive/1.0.0-beta.21155.3, which is a netstandard2.1 library. So I guess that's the minimum? dotnet/interactive has already gone to net5.0, so I think we could bump this from netcoreapp3.1 to net5.0.

Are dependencies allowed?

I assume any dependencies you have outside of your NuGet package that aren't already dependencies of dotnet/interactive need to be included. But as it is now, Microsoft.Data.Analysis.Interactive doesn't have any external dependencies.

Should the extensions deps file be included?

@colombod @brettfo would know.

[ericstj]

I poked around the source and it looks like it just does a LoadFrom: https://github.com/dotnet/interactive/blob/6fd92eb9d72c119b6e7822ca87d96427399a3307/src/Microsoft.DotNet.Interactive/Extensions/AssemblyBasedExtensionLoader.cs#L85

I wonder how that plugin then finds the right copy of M.D.A 🤔 Anyhow, that's not what this comment was about. That's a thought for another day.

[ericstj]

I was hacking around a bit with this and found that you could do this like the following which is marginally better:

    <BeforePack>$(BeforePack);IncludeInteractiveExtension</BeforePack>
  </PropertyGroup>

  <Target Name="IncludeInteractiveExtension">
    <MSBuild Projects="../ext/ext.csproj" Targets="GetTargetPath">
      <Output TaskParameter="TargetOutputs" ItemName="_interactiveExtension" />
    </MSBuild>
    <ItemGroup>
      <Content Include="@(_interactiveExtension)" Pack="True" PackagePath="interactive-extensions/dotnet" />
    </ItemGroup>
  </Target>

This works so long as the interactive extension doesn't cross-target, but it seems like that's a requirement already. If you ever wanted to cross-target you could define a target in the extension project that ran in outer build and invoked the appropriate inner build framework to select the one that goes in the package (and avoid hardcoding the TFM in a different project).

Finally: what actually builds this project during pack? I see that M.D.A can't take a project reference since M.D.A.I itself references M.D.A. If you wanted the build to happen during pack you could change the target above to Build instead of GetTargetPath.

[codecov]

Codecov Report

Merging #5641 (d325692) into master (f93fa09) will decrease coverage by 6.21%.
The diff coverage is 65.35%.

@@            Coverage Diff             @@
##           master    #5641      +/-   ##
==========================================
- Coverage   74.45%   68.24%   -6.22%     
==========================================
  Files        1072     1130      +58     
  Lines      195996   240121   +44125     
  Branches    21547    24920    +3373     
==========================================
+ Hits       145933   163862   +17929     
- Misses      44270    69787   +25517     
- Partials     5793     6472     +679     

Flag	Coverage Δ
Debug	68.24% <65.35%> (-6.22%)	⬇️
production	62.89% <64.01%> (-7.72%)	⬇️
test	89.13% <90.35%> (+1.20%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
....Data.Analysis/DataFrameColumn.BinaryOperations.cs	0.00% <0.00%> (ø)
...FrameColumn.BinaryOperationAPIs.ExplodedColumns.cs	7.57% <ø> (ø)
...lysis/PrimitiveDataFrameColumn.BinaryOperations.cs	39.97% <ø> (ø)
...alysis/PrimitiveDataFrameColumn.BinaryOperators.cs	9.50% <ø> (ø)
...ata.Analysis/PrimitiveDataFrameColumnArithmetic.cs	49.49% <ø> (ø)
...a.Analysis/PrimitiveDataFrameColumnComputations.cs	45.70% <ø> (ø)
...Microsoft.Data.Analysis.Tests/DataFrame.IOTests.cs	98.38% <ø> (ø)
...ysis.Tests/DataFrameColumn.BinaryOperationTests.cs	100.00% <ø> (ø)
...ft.Data.Analysis.Tests/DataFrameTests.IDataView.cs	100.00% <ø> (ø)
...st/Microsoft.Data.Analysis.Tests/DataFrameTests.cs	99.94% <ø> (ø)
... and 128 more

[pgovind]

Alright, I addressed this round of feedback. The only open one left is the Version for DataFrame.

[ericstj]

Tabs hurt my eyes.

[pgovind]

ugh, this setting must be mis-matched locally on my machine between VSCode and VS :\ Will fix in a bit

[pgovind]

Out of curiosity, how did you see tabs in this file in GH? I changed all tabs to spaces (no idea how tabs occurred in the first place) in 4 csprojs in the PR, but unfortunately there is no "Change all tabs to spaces in all files" in VS I think. For now, a solution wide regex search yields no tabs in source code

[ericstj]

It looked too indented then I saw it while trying to select white space.

[eerhardt]

I use CodeFlow to review PRs, and you can tell it to show whitespace. Arrows are tabs, dots are spaces.

[ericstj]

Has codeflow been fixed to work better with Github comment threads? That was a deal breaker in the past.

[eerhardt]

Yeah, it still has some issues with comment threads. Typically I only create new conversations from CodeFlow. And respond to threads in GH. I usually have them both open on 2 different windows. I just like seeing the full file, searching across the change, seeing the tree view of files, etc in CodeFlow. It's a much better experience for me.

[eerhardt]

SA1028 is about code comments not formatting correctly. That seems like something we should be able to fix (either here or in a follow up PR)

[pgovind]

Yup. My plan is to do it in a follow up PR. SA1028 and a bunch of the other warnings are easily fixable

[pgovind]

Let me know if someone would like something else addressed in this PR. Just waiting for a sign off at this point. I think this is good to go in now.

[eerhardt]

Just putting this out there for thought:

We didn't depend on the latest Microsoft.ML.DataView before. We depended on 1.0.0. I wonder if it would be better if we didn't force this dependency to the latest version?

[ericstj]

Is there any unification or breaking change issues? In general we've taken an aggressive update cadence with all packages in dotnet. It helps to ensure folks are patched by default, since Nuget doesn't automatically lift transitive dependencies.

I think someone can still use an old Microsoft.ML with a new DataView right?

[eerhardt]

In general, ML.NET makes heavy usage of InternalsVisibleTo.

However, I looked and I don't think that is true for Microsoft.ML.DataView because at one point we wanted to split it completely apart from Microsoft.ML (we even named it Microsoft.Data.DataView for a time). So I think we should be OK with using a new Microsoft.Data.DataView with an older Microsoft.ML, but I don't think anyone tests it.

[eerhardt]

Just one comment on a dependency version. Other than that, I think we need to wait for green CI. But everything else looks good.

[pgovind]

I think we need to wait for green CI

Wait I don't expect CI to be green here. Or is CI fixed in the repo already and I just haven't rebased yet? From a glance at the CI logs, it looks like the URLs to download some onnx images are wrong/have moved?

[eerhardt]

Wait I don't expect CI to be green here

We need to wait for the repo itself to be working before merging any more changes into it. If you merged this, there is a large potential that your change will break the branch even more.

[pgovind]

Alright, CI is green! Thanks for the reviews everyone and @michaelgsharp for fixing CI