Renamed TlcEnvironment to Console. Also introduced LocalEnvironment #923

Zruty0 · 2018-09-14T23:14:48Z

No description provided.

… AddListener.

Ivanidzo4ka · 2018-09-15T00:17:17Z

test/Microsoft.ML.Tests/Transformers/KeyToVectorEstimatorTests.cs

@@ -215,7 +215,7 @@ private void ValidateMetadata(IDataView result)
        [Fact]
        public void TestCommandLine()
        {
-            using (var env = new TlcEnvironment())
+            using (var env = new ConsoleEnvironment())


using (var env = new ConsoleEnvironment()) [](start = 12, length = 42)

Why I'm even doing this? #Closed

Ivanidzo4ka · 2018-09-15T00:18:18Z

test/Microsoft.ML.Tests/TrainerEstimators/MetalinearEstimators.cs

@@ -97,7 +97,7 @@ public void Pkpd()
        {
            var dataPath = GetDataPath(IrisDataPath);

-            using (var env = new TlcEnvironment())
+            using (var env = new ConsoleEnvironment())


env [](start = 23, length = 3)

you can use Env from base class, but it's unrelated to your PR #Closed

Indeed.

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

I'll fix it.

In reply to: 217869448 [](ancestors = 217869448,217868501)

Ivanidzo4ka · 2018-09-15T00:20:00Z

test/Microsoft.ML.Tests/ImagesTests.cs

@@ -25,7 +25,7 @@ public ImageTests(ITestOutputHelper output) : base(output)
        [Fact]
        public void TestEstimatorChain()
        {
-            using (var env = new TlcEnvironment())
+            using (var env = new ConsoleEnvironment())


env [](start = 23, length = 3)

Want to replace it to Env from TestDataPipeBase? #Closed

Not in this PR

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

Ivanidzo4ka · 2018-09-15T00:27:32Z

        Console.SetOut(_textWriter);

You probably want to hear my valuable opinion?
No?
I'll spill it anyway.

All our tests use XUnit which completely ignores Console, and you have to do this tricks to capture output.
Which is sad.

I would rather had XUnitEnvironment or TestEnvironment which would send output through ITestOutputHelper.
But this is probably unrelated to scope of your PR. #Closed

Refers to: test/Microsoft.ML.StaticPipelineTesting/StaticPipeTests.cs:31 in 28a17e3. [](commit_id = 28a17e3, deletion_comment = False)

Ivanidzo4ka · 2018-09-15T00:31:29Z

src/Microsoft.ML.Data/Utilities/LocalEnvironment.cs

+namespace Microsoft.ML.Runtime.Data
+{
+    using Stopwatch = System.Diagnostics.Stopwatch;
+    public sealed class LocalEnvironment : HostEnvironmentBase<LocalEnvironment>


LocalEnvironment [](start = 24, length = 16)

Maybe SilentEnvironment? Or you somehow can hook up to the messages? #Closed

You can hook up via AddListener<ChannelMessage>

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

Ivanidzo4ka · 2018-09-15T00:33:11Z

src/Microsoft.ML.Core/Data/ProgressReporter.cs

@@ -17,7 +17,7 @@ namespace Microsoft.ML.Runtime.Data
    public static class ProgressReporting
    {
        /// <summary>
-        /// The progress channel for <see cref="TlcEnvironment"/>.
+        /// The progress channel for <see cref="ConsoleEnvironment"/>.


ConsoleEnvironment [](start = 48, length = 18)

Is it really only TlcEnvironment specific or can be used in any HostEnvironmentBase descendant? #Closed

I think it could be used elsewhere, but I'm not sure, and I would rather not try now.

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

Zruty0 · 2018-09-15T00:35:22Z

        Console.SetOut(_textWriter);

Yea, you are right, not it the scope of this PR.
Having a GH issue would help though

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

Refers to: test/Microsoft.ML.StaticPipelineTesting/StaticPipeTests.cs:31 in 28a17e3. [](commit_id = 28a17e3, deletion_comment = False)

Ivanidzo4ka

Ivanidzo4ka · 2018-09-15T00:40:23Z

        Console.SetOut(_textWriter);

#925 sure

In reply to: 421517087 [](ancestors = 421517087,421516389)

Refers to: test/Microsoft.ML.StaticPipelineTesting/StaticPipeTests.cs:31 in 28a17e3. [](commit_id = 28a17e3, deletion_comment = False)

sfilipi

sfilipi · 2018-09-15T03:57:54Z

src/Microsoft.ML.Core/Environment/ConsoleEnvironment.cs

@@ -358,20 +358,36 @@ protected override void DisposeCore()
        private volatile ConsoleWriter _consoleWriter;
        private readonly MessageSensitivity _sensitivityFlags;

-        public TlcEnvironment(int? seed = null, bool verbose = false,
+        /// <summary>
+        /// Create an ML.NET environment for local execution, with console feedback.


ML.NET environmen [](start = 22, length = 17)

add a #Resolved

eerhardt · 2018-09-17T14:31:26Z

test/Microsoft.ML.TestFramework/TestCommandBase.cs

@@ -480,7 +480,7 @@ private string DataArg(string dataPath)

        protected void TestPipeFromModel(string dataPath, OutputPath model)
        {
-            using (var env = new TlcEnvironment(new SysRandom(42)))
+            using (var env = new ConsoleEnvironment(42))


This isn't an equivalent change, right? It used to use SysRandom, but now it will use TauswortheHybrid. #Resolved

it is not. I don't think it matter though

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

eerhardt · 2018-09-17T14:33:30Z

src/Microsoft.ML.Data/Utilities/LocalEnvironment.cs

+    /// <summary>
+    /// An ML.NET environment for local execution.
+    /// </summary>
+    public sealed class LocalEnvironment : HostEnvironmentBase<LocalEnvironment>


We should have some tests that cover this class. #Resolved

I made API scenario tests use it.

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

eerhardt · 2018-09-17T14:34:14Z

src/Microsoft.ML.FastTree/FastTree.cs

@@ -111,7 +111,7 @@ private protected FastTreeTrainerBase(IHostEnvironment env, TArgs args)
            ParallelTraining.InitEnvironment();
            // REVIEW: CLR 4.6 has a bug that is only exposed in Scope, and if we trigger GC.Collect in scope environment
            // with memory consumption more than 5GB, GC get stuck in infinite loop. So for now let's call GC only if we call things from TlcEnvironment.


TlcEnvironment [](start = 138, length = 14)

This comment is now out of date. #Resolved

eerhardt · 2018-09-17T14:35:59Z

src/Microsoft.ML.Data/Utilities/LocalEnvironment.cs

+
+namespace Microsoft.ML.Runtime.Data
+{
+    using Stopwatch = System.Diagnostics.Stopwatch;


What other kind of Stopwatch is there? Can we just have a using System.Diagnostics; at the top? #ByDesign

System.Diagnostics is a pretty big namespace with some names that for whatever reason cleave pretty tightly to common terms in ML (e.g., we have 90% of our code bent around processing instance data, and sure enough we get Process and InstanceData... :P), after several instances of developer confusion, we decided to adopt this practice. My innate tendency would be to keep up our existing practice of, when you want only a stopwatch, just import only that, rather than to relax it just yet. (Or better yet C# could have a less awkward way to import a single class from a namespace.)

Of course we could relax it. That practice dates from the times before we had channels, where literally every component instantiated stopwatches to time themselves, and there were lots of namespace collisions, and so lots of cost. Now that we do have channels, few things produce their own stopwatches -- use of System.Diagnostics is quite limited. So the amount of pain that would be generated now by your suggestion would be much less.

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

yes to the above

In reply to: 218118934 [](ancestors = 218118934,218094176)

eerhardt · 2018-09-17T14:40:42Z

src/Microsoft.ML.Data/Utilities/LocalEnvironment.cs

+            private void ChannelFinished()
+                => Dispatch(this, new ChannelMessage(ChannelMessageKind.Trace, MessageSensitivity.None, "Channel finished. Elapsed { 0:c }.", Watch.Elapsed));
+
+            protected override void DisposeCore()


FYI - I logged #930 to implement the Dispose Pattern correctly on these types. #Resolved

eerhardt · 2018-09-17T14:47:49Z

src/Microsoft.ML.Core/Environment/HostEnvironmentBase.cs

@@ -113,7 +113,7 @@ public interface IMessageDispatcher : IHostEnvironment
    /// AddListener/RemoveListener methods, and exposes the <see cref="ProgressReporting.ProgressTracker"/> to
    /// query progress.
    /// </summary>
-    public abstract class HostEnvironmentBase<TEnv> : ChannelProviderBase, IHostEnvironment, IDisposable, IChannelProvider, IMessageDispatcher
+    public abstract class HostEnvironmentBase<TEnv> : ChannelProviderBase, IHostEnvironment, IChannelProvider, IMessageDispatcher


I think we should get rid of the Dispose method as well.
It feels like we should fix the whole IDisposable concern in 1 change - i.e. remove the Dispose method, remove the "temp file" capability, etc.

Here, we are hanging in limbo where this type doesn't implement IDisposable, but it has a Dispose method, and it holds onto Disposable objects (the IFileHandle instances). #Resolved

Yes, let's separate the change and get rid of temp files later

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

…into feature/host-env

Rename TLC to Console. Added Local that has no default output, but an…

28a17e3

… AddListener.

Zruty0 requested review from Ivanidzo4ka, eerhardt, TomFinley and yaeldekel September 14, 2018 23:14

Ivanidzo4ka reviewed Sep 15, 2018

View reviewed changes

Ivanidzo4ka approved these changes Sep 15, 2018

View reviewed changes

Added comments and a non-generic listener class.

dbe5a71

sfilipi approved these changes Sep 15, 2018

View reviewed changes

sfilipi reviewed Sep 15, 2018

View reviewed changes

eerhardt reviewed Sep 17, 2018

View reviewed changes

Pete Luferenko added 4 commits September 17, 2018 11:29

Merge remote-tracking branch 'upstream/master' into feature/host-env

047f90a

Merge remote-tracking branch 'upstream/master' into feature/host-env

f9a91fa

Merge branch 'feature/host-env' of github.com:Zruty0/machinelearning …

10a8a68

…into feature/host-env

PR comments

0f6bca4

Zruty0 merged commit 160b0df into dotnet:master Sep 17, 2018

Zruty0 deleted the feature/host-env branch September 17, 2018 21:15

eerhardt mentioned this pull request Sep 20, 2018

Need to rename TlcEnvironment #541

Closed

sfilipi mentioned this pull request Oct 24, 2018

Printing training statistics by default discussion #1367

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renamed TlcEnvironment to Console. Also introduced LocalEnvironment #923

Renamed TlcEnvironment to Console. Also introduced LocalEnvironment #923

Zruty0 commented Sep 14, 2018

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Zruty0 Sep 15, 2018

sfilipi Sep 15, 2018

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Zruty0 Sep 15, 2018

Ivanidzo4ka commented Sep 15, 2018 •

edited

Loading

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Zruty0 Sep 15, 2018

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Zruty0 Sep 15, 2018

Zruty0 commented Sep 15, 2018

Ivanidzo4ka left a comment

Ivanidzo4ka commented Sep 15, 2018

sfilipi left a comment

sfilipi Sep 15, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

Zruty0 Sep 17, 2018

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

Zruty0 Sep 17, 2018

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

TomFinley Sep 17, 2018

Zruty0 Sep 17, 2018

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

Zruty0 Sep 17, 2018

Renamed TlcEnvironment to Console. Also introduced LocalEnvironment #923

Renamed TlcEnvironment to Console. Also introduced LocalEnvironment #923

Conversation

Zruty0 commented Sep 14, 2018

Ivanidzo4ka Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Sep 15, 2018 • edited Loading

Ivanidzo4ka Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 commented Sep 15, 2018

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Sep 15, 2018

sfilipi left a comment

Choose a reason for hiding this comment

sfilipi Sep 15, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

eerhardt Sep 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Sep 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Sep 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

eerhardt Sep 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Sep 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

eerhardt Sep 17, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Ivanidzo4ka commented Sep 15, 2018 •

edited

Loading

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

Ivanidzo4ka Sep 15, 2018 •

edited

Loading

sfilipi Sep 15, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading

eerhardt Sep 17, 2018 •

edited by Zruty0

Loading