Internalize IDataLoader #2309
Internalize IDataLoader #2309
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2309 +/- ##
==========================================
+ Coverage 71.16% 71.19% +0.02%
==========================================
Files 780 780
Lines 140340 140328 -12
Branches 16048 16045 -3
==========================================
+ Hits 99879 99908 +29
+ Misses 36011 35968 -43
- Partials 4450 4452 +2
|
Ivanidzo4ka
changed the title
|
Thank you Ivan. What of the BinaryLoader? Should that remain public as well? |
|
Funny. I expect to get bunch of exceptions from any class which implements IDataLoader and public, but apparently that's not the case. In reply to: 458766322 [](ancestors = 458766322) |
Right. So whether a method is public or not versus whether it adheres to the contract are, in some sense, independent. So you can have an internal interface, a public class can adhere to that interface with public methods. This FYI is why we often have the additional work, if the class must remain public, that its implementing methods become explicit, if that makes sense? For now, it would be preferable if, as I think you have done, that the implementing classes themselves become internal, since we would want them to adhere not to the |
| @@ -193,12 +196,13 @@ public IEnumerable<string> ParseValues(string path) | |||
|
|
|||
| [TlcModule.Component(Name = ParquetPartitionedPathParser.LoadName, FriendlyName = ParquetPartitionedPathParser.UserName, | |||
| Desc = ParquetPartitionedPathParser.Summary, Alias = ParquetPartitionedPathParser.ShortName)] | |||
| public class ParquetPartitionedPathParserFactory : IPartitionedPathParserFactory | |||
| internal class ParquetPartitionedPathParserFactory : IPartitionedPathParserFactory | |||
There was a problem hiding this comment.
ParquetPartitionedPathParserFactory [](start = 19, length = 35)
Huh... weird. Not your code I know, just curious, is it obvious why this isn't living off in Parquet land? #Resolved
There was a problem hiding this comment.
I don't know, and from your tone it looks like you want me to move them to Parquet project. Your wish is my command master.
In reply to: 252374825 [](ancestors = 252374825)
There was a problem hiding this comment.
Well, that would certainly be very nice, thank you @Ivanidzo4ka. 😄
| @@ -126,7 +126,8 @@ public static IDataView LoadTransforms(this IHostEnvironment env, Stream modelSt | |||
| /// <summary> | |||
| /// Creates a data loader from the arguments object. | |||
| /// </summary> | |||
| public static IDataLoader CreateLoader<TArgs>(this IHostEnvironment env, TArgs arguments, IMultiStreamSource files) | |||
| [BestFriend] | |||
| internal static IDataLoader CreateLoader<TArgs>(this IHostEnvironment env, TArgs arguments, IMultiStreamSource files) | |||
There was a problem hiding this comment.
CreateLoader [](start = 36, length = 12)
Thanks Ivan. I wouldn't necessarily do this now, but this entire assembly seems like a lot of stuff we do not want public. But not as part of this PR. #Resolved
There was a problem hiding this comment.
It's hard to have internal interface IDataLoader and keep this method public.
Compiler complains on return type is less accessible than method.
In reply to: 252375310 [](ancestors = 252375310)
There was a problem hiding this comment.
Oh what I meant was the whole class. But like I said not part of this PR.
But we will make it internal anyways as part of the issue that cleans up the transformers. In reply to: 459065845 [](ancestors = 459065845) Refers to: src/Microsoft.ML.Data/Transforms/ValueToKeyMappingTransformer.cs:111 in 66de22e. [](commit_id = 66de22e, deletion_comment = False) |
Yeah, I would prefer to not diverge from intent of this PR In reply to: 459066115 [](ancestors = 459066115,459065845) Refers to: src/Microsoft.ML.Data/Transforms/ValueToKeyMappingTransformer.cs:111 in 66de22e. [](commit_id = 66de22e, deletion_comment = False) |
| @@ -276,8 +280,7 @@ public void Save(ModelSaveContext ctx) | |||
| ctx.SaveString(sb.ToString()); | |||
| }; | |||
| } | |||
|
|
|||
| public IEnumerable<PartitionedFileLoader.Column> ParseColumns(string path) | |||
| IEnumerable<PartitionedFileLoader.Column> IPartitionedPathParser.ParseColumns(string path) | |||
There was a problem hiding this comment.
IEnumerable<PartitionedFileLoader.Column> IPartitionedPathParser.ParseColumns(string path) [](start = 8, length = 90)
Why this change if the outer class is internal? Also you didn't change ParseColumns above, which makes me thing it does not need to be changed.
There was a problem hiding this comment.
I made this change first, and only after that made whole class internal. Thanks for the catch!
In reply to: 252397204 [](ancestors = 252397204)
| @@ -362,7 +362,7 @@ internal bool TryUnparse(StringBuilder sb) | |||
| } | |||
| } | |||
|
|
|||
| public sealed class Arguments | |||
| internal sealed class Arguments | |||
There was a problem hiding this comment.
internal [](start = 8, length = 8)
For consistency, I would either make this public and only make specific field internal, or make the Arguments internal in the other transforms too. #Resolved
There was a problem hiding this comment.
In other places we use Arguments classes as EntryPoint arguments. I don't want to get into business of cleaning entrypoints in this PR.
For this transform we don't have entrypoint, so I prefer to just hide it completely.
In reply to: 252398379 [](ancestors = 252398379)
There was a problem hiding this comment.
ghost
locked as resolved and limited conversation to collaborators
Towards ##1995