Removed Read methods taking string parameters from TextLoader (#1797) #2323

PaulTFreedman · 2019-01-30T16:47:34Z

Unsure about the naming of LocalPathReader but I kept it the same due to the requirement that it be namespaced to Microsoft.ML - DataViewExtensions already exists and is in the Microsoft.ML.Data namespace.

Fixes #1797.

…#1797)

dnfclas · 2019-01-30T16:47:46Z

All CLA requirements met.

codecov · 2019-01-30T17:28:15Z

Codecov Report

Merging #2323 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #2323     +/-   ##
=========================================
+ Coverage   71.15%   71.25%   +0.1%     
=========================================
  Files         779      785      +6     
  Lines      140311   140785    +474     
  Branches    16047    16088     +41     
=========================================
+ Hits        99838   100319    +481     
+ Misses      36021    36012      -9     
- Partials     4452     4454      +2

Flag	Coverage Δ
#Debug	`71.25% <100%> (+0.1%)`	⬆️
#production	`67.61% <100%> (+0.03%)`	⬆️
#test	`85.3% <ø> (+0.21%)`	⬆️

Ivanidzo4ka · 2019-01-31T02:42:05Z

test/Microsoft.ML.Predictor.Tests/Microsoft.ML.Predictor.Tests.csproj

@@ -11,6 +11,7 @@
    <ProjectReference Include="..\..\src\Microsoft.ML.LightGBM\Microsoft.ML.LightGBM.csproj" />
    <ProjectReference Include="..\..\src\Microsoft.ML.PCA\Microsoft.ML.PCA.csproj" />
    <ProjectReference Include="..\..\src\Microsoft.ML.StandardLearners\Microsoft.ML.StandardLearners.csproj" />
+    <ProjectReference Include="..\..\src\Microsoft.ML.StaticPipe\Microsoft.ML.StaticPipe.csproj" />


StaticPipe [](start = 78, length = 10)

This is not a good sign.
In current moment we prefer to separate our StaticPipe project from rest of the code.

I would suggest to split LocalPathReader into two classes. One should stay next to TextLoader.cs and have just Read method without <TShape>.
Second should remain in StaticPipe project, and have Read method with <TShape>.

Just to be clear on your meaning, when you say with/without "." you're talking about calling something like reader.Read(...) vs just Read(...)?

Sorry for confusion, I forgot what web interface doesn't gracefully handles tags, and you need to escape them all the time.
I was talking about public static DataView<TShape> Read<TShape> that thing should remain in StaticPipe project.

If I correctly understand you code and intention you removing public IDataView Read(string path) from TextLoader but since LocalPathReader contains extension method on IDataReader<IMultiStreamSource> reader all our code continue to work since we fall back to that extension.

This is great, it just that extension shouldn't be part of StaticPipe project. I would prefer it to remain in Microsoft.ML.Data project.
In same time second extension method which works on top of DataReader<IMultiStreamSource, TShape> should remain to be inside StaticPipe project.

Does my explanation make sense?

I think so, the earlier message makes a lot more sense now you've added in the tags! I've updated the pull request, does it look better now?

Ivanidzo4ka · 2019-01-31T02:44:02Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

@@ -1312,10 +1312,6 @@ public void Save(ModelSaveContext ctx)

        public IDataView Read(IMultiStreamSource source) => new BoundLoader(this, source);

-        public IDataView Read(string path) => Read(new MultiFileSource(path));
-
-        public IDataView Read(params string[] path) => Read(new MultiFileSource(path));


I think we prefer to have two methods because params is a bit slow one, and for performance we prefer to have one method for single file case (which is like 99%) and one for case with multiple file paths. #Closed

Ivanidzo4ka · 2019-01-31T02:45:05Z

@PaulTFreedman thank you for your contribution! I left few comments, can you take care of them?

…L.StaticPipe

Ivanidzo4ka · 2019-02-02T17:29:42Z

using Microsoft.Data.DataView;

please add our standard header to the file.

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

Refers to: src/Microsoft.ML.Data/DataLoadSave/DataReaderExtensions.cs:1 in 0a4cb1f. [](commit_id = 0a4cb1f, deletion_comment = False)

Ivanidzo4ka · 2019-02-02T17:30:19Z

src/Microsoft.ML.StaticPipe/LocalPathReader.cs

+using Microsoft.ML.Data;
+using Microsoft.ML.StaticPipe;
+
+namespace Microsoft.ML


ML [](start = 20, length = 2)

I would leave it in ML.StaticPipe namespace.

Ivanidzo4ka · 2019-02-02T17:30:47Z

using Microsoft.ML.Data;

please add header (see same comments for details)

Refers to: src/Microsoft.ML.StaticPipe/LocalPathReader.cs:1 in 0a4cb1f. [](commit_id = 0a4cb1f, deletion_comment = False)

Ivanidzo4ka

Looking good. Few small things left.

…ataReader extensions (dotnet#1797)

PaulTFreedman · 2019-02-03T22:06:05Z

I'm not sure why it fails the code coverage part, it happens locally too but the output doesn't give me any useful error message. I can try to debug when I have time but if someone else has any ideas that would be very helpful.

Ivanidzo4ka · 2019-02-04T01:47:56Z

@PaulTFreedman it looks fine now. At least I can see updated post from coverage service, and all builds in passed state. I just need to find second person to review this change. I'll make sure someone will take a look during Monday.

artidoro

artidoro · 2019-02-04T23:41:39Z

Thank you!

Removed Read methods taking string parameters from TextLoader (dotnet…

1118e52

…#1797)

Ivanidzo4ka reviewed Jan 31, 2019

View reviewed changes

Split DataReader extensions between Microsoft.ML.Data and Microsoft.M…

0a4cb1f

…L.StaticPipe

Ivanidzo4ka reviewed Feb 2, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 2, 2019

View reviewed changes

Ivanidzo4ka requested review from artidoro and TomFinley February 2, 2019 17:38

Added standard header to new files and changed namespace for static D…

54c5200

…ataReader extensions (dotnet#1797)

Ivanidzo4ka requested review from sfilipi and abgoswam February 3, 2019 20:27

artidoro approved these changes Feb 4, 2019

View reviewed changes

artidoro merged commit c544d51 into dotnet:master Feb 4, 2019

ghost locked as resolved and limited conversation to collaborators Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed Read methods taking string parameters from TextLoader (#1797) #2323

Removed Read methods taking string parameters from TextLoader (#1797) #2323

PaulTFreedman commented Jan 30, 2019 •

edited by artidoro

Loading

dnfclas commented Jan 30, 2019 •

edited

Loading

codecov bot commented Jan 30, 2019 •

edited

Loading

Ivanidzo4ka Jan 31, 2019 •

edited

Loading

PaulTFreedman Jan 31, 2019

Ivanidzo4ka Jan 31, 2019

PaulTFreedman Feb 2, 2019

Ivanidzo4ka Jan 31, 2019 •

edited

Loading

Ivanidzo4ka commented Jan 31, 2019

Ivanidzo4ka commented Feb 2, 2019

Ivanidzo4ka Feb 2, 2019

Ivanidzo4ka commented Feb 2, 2019

Ivanidzo4ka left a comment

PaulTFreedman commented Feb 3, 2019

Ivanidzo4ka commented Feb 4, 2019

artidoro left a comment

artidoro commented Feb 4, 2019

Removed Read methods taking string parameters from TextLoader (#1797) #2323

Removed Read methods taking string parameters from TextLoader (#1797) #2323

Conversation

PaulTFreedman commented Jan 30, 2019 • edited by artidoro Loading

dnfclas commented Jan 30, 2019 • edited Loading

codecov bot commented Jan 30, 2019 • edited Loading

Codecov Report

Ivanidzo4ka Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

PaulTFreedman Jan 31, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Jan 31, 2019

Choose a reason for hiding this comment

PaulTFreedman Feb 2, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Jan 31, 2019

Ivanidzo4ka commented Feb 2, 2019

Ivanidzo4ka Feb 2, 2019

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 2, 2019

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

PaulTFreedman commented Feb 3, 2019

Ivanidzo4ka commented Feb 4, 2019

artidoro left a comment

Choose a reason for hiding this comment

artidoro commented Feb 4, 2019

PaulTFreedman commented Jan 30, 2019 •

edited by artidoro

Loading

dnfclas commented Jan 30, 2019 •

edited

Loading

codecov bot commented Jan 30, 2019 •

edited

Loading

Ivanidzo4ka Jan 31, 2019 •

edited

Loading

Ivanidzo4ka Jan 31, 2019 •

edited

Loading