Skip to content

Comments

Remove ConcurrencyFactor from IHostEnvironment#2846

Merged
artidoro merged 7 commits intodotnet:masterfrom
artidoro:conc
Mar 7, 2019
Merged

Remove ConcurrencyFactor from IHostEnvironment#2846
artidoro merged 7 commits intodotnet:masterfrom
artidoro:conc

Conversation

@artidoro
Copy link
Contributor

@artidoro artidoro commented Mar 5, 2019

Fixes #2051.

In this PR I remove ConcurrencyFactor from IHostEnvironment and interfaces and classes deriving/implementing it.

I had to skip a RandomPredictor as I am not sure how I can require a single threaded behavior without the ConcurrencyFactor for this specific trainer.

@artidoro
Copy link
Contributor Author

artidoro commented Mar 5, 2019

Note that the first and second commit contain the most important logical changes the rest are fixes to tests and samples to make sure they compile.

@artidoro artidoro self-assigned this Mar 5, 2019
@artidoro artidoro added the API Issues pertaining the friendly API label Mar 5, 2019
@artidoro artidoro added this to the 0319 milestone Mar 5, 2019
@artidoro artidoro changed the title WIP: Remove ConcurrencyFactor from IHostEnvironment Remove ConcurrencyFactor from IHostEnvironment Mar 5, 2019
@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #2846 into master will decrease coverage by <.01%.
The diff coverage is 94.92%.

@@            Coverage Diff             @@
##           master    #2846      +/-   ##
==========================================
- Coverage   71.69%   71.69%   -0.01%     
==========================================
  Files         811      811              
  Lines      142546   142494      -52     
  Branches    16125    16112      -13     
==========================================
- Hits       102193   102155      -38     
+ Misses      35924    35918       -6     
+ Partials     4429     4421       -8
Flag Coverage Δ
#Debug 71.69% <94.92%> (-0.01%) ⬇️
#production 67.91% <92%> (-0.01%) ⬇️
#test 85.91% <95.57%> (ø) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.Core/Data/IHostEnvironment.cs 95.12% <ø> (ø) ⬆️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs 73.67% <ø> (-0.09%) ⬇️
.../Standard/LogisticRegression/LbfgsPredictorBase.cs 71.26% <ø> (+0.96%) ⬆️
...t/Microsoft.ML.Benchmarks/PredictionEngineBench.cs 0% <0%> (ø) ⬆️
...c/Microsoft.ML.FastTree/SumupPerformanceCommand.cs 0% <0%> (ø) ⬆️
src/Microsoft.ML.FastTree/GamTrainer.cs 89.83% <0%> (-0.22%) ⬇️
...st/Microsoft.ML.Tests/Scenarios/TensorflowTests.cs 100% <100%> (ø) ⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs 100% <100%> (ø) ⬆️
...st/Microsoft.ML.Tests/Scenarios/ClusteringTests.cs 100% <100%> (ø) ⬆️
...osoft.ML.TimeSeries.Tests/TimeSeriesStaticTests.cs 98.56% <100%> (ø) ⬆️
... and 48 more

@TomFinley
Copy link
Contributor

TomFinley commented Mar 5, 2019

I had to skip a RandomPredictor as I am not sure how I can require a single threaded behavior without the ConcurrencyFactor for this specific trainer.

Actually that's interesting. We need to hide this for now, just as a we had to hide GenerateNumberTransform. (Not necessarily part of your PR, but if you wanted to do it it wouldn't be that hard, we'd just need to hide RandomModelParameters and RandomTrainer.) RandomModelParameters claims it implements the internal interface IValueMapperDist, which means that it is claiming (via our infrastructure) that it is possible to express as an IRowToRowMapper, but it can't since the outputs are not independent.

Edit: To address this problem I've introduced issue #2848 and PR #2849. It had somehow escaped my notice that we had a random model just lying around claiming to be a value mapper. (And thereby introducing the bug you observed.) That's obviously not going to fly... #Resolved

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

:shipit:

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Mar 6, 2019

        //      The coordinates of centroid 0 are: (26, 6, 1)

I would double check output in samples.
Or just set numThreads in KMeans to 1. #Resolved


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Clustering/KMeans.cs:45 in a8c7541. [](commit_id = a8c7541, deletion_comment = False)

@Ivanidzo4ka
Copy link
Contributor

        //      The coordinates of centroid 0 are: (26, 6, 1)

Please validate what output in samples are same across all files in this PR


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


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Clustering/KMeans.cs:45 in a8c7541. [](commit_id = a8c7541, deletion_comment = False)

@@ -59,14 +59,7 @@ public interface IHostEnvironment : IChannelProvider, IProgressChannelProvider
/// <summary>
/// Create a host with the given registration name.
/// </summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain what seed and verbose are.

@artidoro artidoro merged commit 0075757 into dotnet:master Mar 7, 2019
@artidoro artidoro deleted the conc branch March 13, 2019 17:55
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

API Issues pertaining the friendly API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants