Use SweepablePipeline - #6285

Merged
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer
Aug 25, 2022
Merged

Use SweepablePipeline#6285
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer

Conversation

@LittleLittleCloud

@LittleLittleCloudLittleLittleCloud commented Aug 17, 2022

Copy link
Copy Markdown
Member

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Check out the spec for SweepablePipeline in #6218

SweepablePipeline is a combination of MultiModelPipeline and SweepableEstimatorPipeline, which supports a tree-like structure pipeline and support estimator-level search space using nested search space.

In another world, SweepablePipeline puts estimator candidates as part of its search space and makes it transparent to tuner. In this way, it decouples tuners from the detailed implementation of pipelines or trainers, and replacing them with Parameter and SearchSpace. The hyper-parameter optimization process, with the help of SweepablePipeline, can be simplified to the following 3 steps

  • ITuner sample parameter from search space
  • ITrialRunner train model and calculate score from parameter
  • ITuner update associated parameter with score.

Also, it provides a uniform way to create pipeline that includes multiple estimator candidates with search space.

And with this PR, the class that construct AutoML.Net Sweepable API is simplified to

  • ISweepable
    • SweepableEstimator: Estimator with search space
    • SweepablePipeline pipeline with search space

@LittleLittleCloudLittleLittleCloud changed the title [wip] Use SweepablePipelineUse SweepablePipelineAug 18, 2022
@LittleLittleCloudLittleLittleCloud changed the title Use SweepablePipeline[wip] - Use SweepablePipelineAug 18, 2022
@codecov

codecovBot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6285 (4281116) into main (8589d25) will increase coverage by 0.05%.
The diff coverage is 99.65%.

Additional details and impacted files
@@ Coverage Diff @@## main #6285 +/- ##
==========================================
+ Coverage 68.52% 68.58% +0.05% 
==========================================
Files 1170 1170 Lines 246931 247158 +227 Branches 25669 25675 +6 ==========================================
+ Hits 169220 169512 +292 + Misses 70961 70905 -56 + Partials 6750 6741 -9 
FlagCoverage Δ
Debug68.58% <99.65%> (+0.05%)⬆️
production63.01% <ø> (+0.01%)⬆️
test89.09% <99.65%> (+0.11%)⬆️

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

Impacted FilesCoverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs83.04% <98.64%> (+6.72%)⬆️
...t/Microsoft.ML.AutoML.Tests/AutoFeaturizerTests.cs92.45% <100.00%> (+0.96%)⬆️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs100.00% <100.00%> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs97.84% <100.00%> (+16.78%)⬆️
...icrosoft.ML.AutoML.Tests/SweepableExtensionTest.cs96.00% <100.00%> (+1.26%)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.38% <0.00%> (-0.15%)⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs77.94% <0.00%> (ø)
src/Microsoft.ML.Data/Data/Conversion.cs79.98% <0.00%> (+0.09%)⬆️
src/Microsoft.ML.SearchSpace/SearchSpace.cs72.01% <0.00%> (+0.45%)⬆️
... and 6 more

@LittleLittleCloudLittleLittleCloud changed the title [wip] - Use SweepablePipelineUse SweepablePipelineAug 22, 2022

public static AutoMLExperiment SetBinaryClassificationMetric(this AutoMLExperiment experiment, BinaryClassificationMetric metric, string labelColumn = "label", string predictedColumn = "PredictedLabel")
{
var metricManager = new BinaryMetricManager(metric, predictedColumn, labelColumn);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

predictedColumn, labelColumn

should we flip the order of these parameters to be consistent with the rest of APIs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

labelColumn should come before predictedColumn in order to be consistent with context.Binary.Evaluation api, I'll update BinaryMetricManager though

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved


pipeline = pipeline.Append(Context.Auto().Featurizer(trainData, columnInformation, Features));
return pipeline.Append(Context.Auto().BinaryClassification(label, useSdca: useSdca, useFastTree: useFastTree, useLgbm: useLgbm, useLbfgs: uselbfgs, useFastForest: useFastForest, featureColumnName: Features));
throw new ArgumentException("IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

nit: I am seeing this message will not be clear if I see it thrown. Maybe you can modify it a little to tell something like,

$"The runner metric manager is of type {_metricManager.GetType()} which expected to be of type BinaryMetricManage"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

{
var label = columnInformation.LabelColumnName;
_experiment.SetEvaluateMetric(Settings.OptimizingMetric, label);
TrialResultMonitor<MulticlassClassificationMetrics> monitor = null;

@tarekghtarekghAug 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TrialResultMonitor monitor = null;

nit: maybe better move this line down before _experiment.SetMonitor line?
This comment apply to similar places.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

}
}

throw new ArgumentException("IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

ditto.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

@tarekgh

Copy link
Copy Markdown
Member
 else

nit: you don't need the else here.


Refers to: src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:256 in 6de7519. [](commit_id = 6de7519, deletion_comment = False)

monitor.ReportFailTrial(setting, ex);
throw;
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else

else not needed here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That else will be hit when you get a training error but has a successful trial result(therefore _bestTrialResult is not null). In which case the current best result will be returned instead.

This is to avoid the case of losing all available trial results when encountering an unfatal error, like OOM or so. The more reliable way of doing that is, of course, detecting if exception from trial is fatal or not and continue training if the exception is not fatal. But in that case we need to cover all unfatal cases which is almost impossible and unnecessary. So as a step back, in order not to loss current training result, AutoMLExperiment simply 1) prints out exception and 2) return _currentBestTrial if there's any when encountering any exception. Only when there's no completed trial will AutoMLExperiment throws an exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The if block is throwing any way. so no need to have explicit else.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

OIC

Comment threadsrc/Microsoft.ML.AutoML/Tuner/EciCfoTuner.cs Outdated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added minor comments. In general the change LGTM as you explained it to me.

@LittleLittleCloud

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
LittleLittleCloud merged commit 9652e59 into dotnet:mainAug 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleLittleCloud@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Use SweepablePipeline - #6285

Merged
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer
Aug 25, 2022
Merged

Use SweepablePipeline#6285
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer

Conversation

@LittleLittleCloud

@LittleLittleCloudLittleLittleCloud commented Aug 17, 2022

Copy link
Copy Markdown
Member

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Check out the spec for SweepablePipeline in #6218

SweepablePipeline is a combination of MultiModelPipeline and SweepableEstimatorPipeline, which supports a tree-like structure pipeline and support estimator-level search space using nested search space.

In another world, SweepablePipeline puts estimator candidates as part of its search space and makes it transparent to tuner. In this way, it decouples tuners from the detailed implementation of pipelines or trainers, and replacing them with Parameter and SearchSpace. The hyper-parameter optimization process, with the help of SweepablePipeline, can be simplified to the following 3 steps

  • ITuner sample parameter from search space
  • ITrialRunner train model and calculate score from parameter
  • ITuner update associated parameter with score.

Also, it provides a uniform way to create pipeline that includes multiple estimator candidates with search space.

And with this PR, the class that construct AutoML.Net Sweepable API is simplified to

  • ISweepable
    • SweepableEstimator: Estimator with search space
    • SweepablePipeline pipeline with search space

@LittleLittleCloudLittleLittleCloud changed the title [wip] Use SweepablePipelineUse SweepablePipelineAug 18, 2022
@LittleLittleCloudLittleLittleCloud changed the title Use SweepablePipeline[wip] - Use SweepablePipelineAug 18, 2022
@codecov

codecovBot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6285 (4281116) into main (8589d25) will increase coverage by 0.05%.
The diff coverage is 99.65%.

Additional details and impacted files
@@ Coverage Diff @@## main #6285 +/- ##
==========================================
+ Coverage 68.52% 68.58% +0.05% 
==========================================
Files 1170 1170 Lines 246931 247158 +227 Branches 25669 25675 +6 ==========================================
+ Hits 169220 169512 +292 + Misses 70961 70905 -56 + Partials 6750 6741 -9 
FlagCoverage Δ
Debug68.58% <99.65%> (+0.05%)⬆️
production63.01% <ø> (+0.01%)⬆️
test89.09% <99.65%> (+0.11%)⬆️

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

Impacted FilesCoverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs83.04% <98.64%> (+6.72%)⬆️
...t/Microsoft.ML.AutoML.Tests/AutoFeaturizerTests.cs92.45% <100.00%> (+0.96%)⬆️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs100.00% <100.00%> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs97.84% <100.00%> (+16.78%)⬆️
...icrosoft.ML.AutoML.Tests/SweepableExtensionTest.cs96.00% <100.00%> (+1.26%)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.38% <0.00%> (-0.15%)⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs77.94% <0.00%> (ø)
src/Microsoft.ML.Data/Data/Conversion.cs79.98% <0.00%> (+0.09%)⬆️
src/Microsoft.ML.SearchSpace/SearchSpace.cs72.01% <0.00%> (+0.45%)⬆️
... and 6 more

@LittleLittleCloudLittleLittleCloud changed the title [wip] - Use SweepablePipelineUse SweepablePipelineAug 22, 2022

public static AutoMLExperiment SetBinaryClassificationMetric(this AutoMLExperiment experiment, BinaryClassificationMetric metric, string labelColumn = "label", string predictedColumn = "PredictedLabel")
{
var metricManager = new BinaryMetricManager(metric, predictedColumn, labelColumn);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

predictedColumn, labelColumn

should we flip the order of these parameters to be consistent with the rest of APIs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

labelColumn should come before predictedColumn in order to be consistent with context.Binary.Evaluation api, I'll update BinaryMetricManager though

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved


pipeline = pipeline.Append(Context.Auto().Featurizer(trainData, columnInformation, Features));
return pipeline.Append(Context.Auto().BinaryClassification(label, useSdca: useSdca, useFastTree: useFastTree, useLgbm: useLgbm, useLbfgs: uselbfgs, useFastForest: useFastForest, featureColumnName: Features));
throw new ArgumentException("IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

nit: I am seeing this message will not be clear if I see it thrown. Maybe you can modify it a little to tell something like,

$"The runner metric manager is of type {_metricManager.GetType()} which expected to be of type BinaryMetricManage"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

{
var label = columnInformation.LabelColumnName;
_experiment.SetEvaluateMetric(Settings.OptimizingMetric, label);
TrialResultMonitor<MulticlassClassificationMetrics> monitor = null;

@tarekghtarekghAug 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TrialResultMonitor monitor = null;

nit: maybe better move this line down before _experiment.SetMonitor line?
This comment apply to similar places.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

}
}

throw new ArgumentException("IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

ditto.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

@tarekgh

Copy link
Copy Markdown
Member
 else

nit: you don't need the else here.


Refers to: src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:256 in 6de7519. [](commit_id = 6de7519, deletion_comment = False)

monitor.ReportFailTrial(setting, ex);
throw;
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else

else not needed here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That else will be hit when you get a training error but has a successful trial result(therefore _bestTrialResult is not null). In which case the current best result will be returned instead.

This is to avoid the case of losing all available trial results when encountering an unfatal error, like OOM or so. The more reliable way of doing that is, of course, detecting if exception from trial is fatal or not and continue training if the exception is not fatal. But in that case we need to cover all unfatal cases which is almost impossible and unnecessary. So as a step back, in order not to loss current training result, AutoMLExperiment simply 1) prints out exception and 2) return _currentBestTrial if there's any when encountering any exception. Only when there's no completed trial will AutoMLExperiment throws an exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The if block is throwing any way. so no need to have explicit else.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

OIC

Comment threadsrc/Microsoft.ML.AutoML/Tuner/EciCfoTuner.cs Outdated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added minor comments. In general the change LGTM as you explained it to me.

@LittleLittleCloud

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
LittleLittleCloud merged commit 9652e59 into dotnet:mainAug 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleLittleCloud@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use SweepablePipeline - #6285

Merged
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer
Aug 25, 2022
Merged

Use SweepablePipeline#6285
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer

Conversation

@LittleLittleCloud

@LittleLittleCloudLittleLittleCloud commented Aug 17, 2022

Copy link
Copy Markdown
Member

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Check out the spec for SweepablePipeline in #6218

SweepablePipeline is a combination of MultiModelPipeline and SweepableEstimatorPipeline, which supports a tree-like structure pipeline and support estimator-level search space using nested search space.

In another world, SweepablePipeline puts estimator candidates as part of its search space and makes it transparent to tuner. In this way, it decouples tuners from the detailed implementation of pipelines or trainers, and replacing them with Parameter and SearchSpace. The hyper-parameter optimization process, with the help of SweepablePipeline, can be simplified to the following 3 steps

  • ITuner sample parameter from search space
  • ITrialRunner train model and calculate score from parameter
  • ITuner update associated parameter with score.

Also, it provides a uniform way to create pipeline that includes multiple estimator candidates with search space.

And with this PR, the class that construct AutoML.Net Sweepable API is simplified to

  • ISweepable
    • SweepableEstimator: Estimator with search space
    • SweepablePipeline pipeline with search space

@LittleLittleCloudLittleLittleCloud changed the title [wip] Use SweepablePipelineUse SweepablePipelineAug 18, 2022
@LittleLittleCloudLittleLittleCloud changed the title Use SweepablePipeline[wip] - Use SweepablePipelineAug 18, 2022
@codecov

codecovBot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6285 (4281116) into main (8589d25) will increase coverage by 0.05%.
The diff coverage is 99.65%.

Additional details and impacted files
@@ Coverage Diff @@## main #6285 +/- ##
==========================================
+ Coverage 68.52% 68.58% +0.05% 
==========================================
Files 1170 1170 Lines 246931 247158 +227 Branches 25669 25675 +6 ==========================================
+ Hits 169220 169512 +292 + Misses 70961 70905 -56 + Partials 6750 6741 -9 
FlagCoverage Δ
Debug68.58% <99.65%> (+0.05%)⬆️
production63.01% <ø> (+0.01%)⬆️
test89.09% <99.65%> (+0.11%)⬆️

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

Impacted FilesCoverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs83.04% <98.64%> (+6.72%)⬆️
...t/Microsoft.ML.AutoML.Tests/AutoFeaturizerTests.cs92.45% <100.00%> (+0.96%)⬆️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs100.00% <100.00%> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs97.84% <100.00%> (+16.78%)⬆️
...icrosoft.ML.AutoML.Tests/SweepableExtensionTest.cs96.00% <100.00%> (+1.26%)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.38% <0.00%> (-0.15%)⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs77.94% <0.00%> (ø)
src/Microsoft.ML.Data/Data/Conversion.cs79.98% <0.00%> (+0.09%)⬆️
src/Microsoft.ML.SearchSpace/SearchSpace.cs72.01% <0.00%> (+0.45%)⬆️
... and 6 more

@LittleLittleCloudLittleLittleCloud changed the title [wip] - Use SweepablePipelineUse SweepablePipelineAug 22, 2022

public static AutoMLExperiment SetBinaryClassificationMetric(this AutoMLExperiment experiment, BinaryClassificationMetric metric, string labelColumn = "label", string predictedColumn = "PredictedLabel")
{
var metricManager = new BinaryMetricManager(metric, predictedColumn, labelColumn);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

predictedColumn, labelColumn

should we flip the order of these parameters to be consistent with the rest of APIs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

labelColumn should come before predictedColumn in order to be consistent with context.Binary.Evaluation api, I'll update BinaryMetricManager though

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved


pipeline = pipeline.Append(Context.Auto().Featurizer(trainData, columnInformation, Features));
return pipeline.Append(Context.Auto().BinaryClassification(label, useSdca: useSdca, useFastTree: useFastTree, useLgbm: useLgbm, useLbfgs: uselbfgs, useFastForest: useFastForest, featureColumnName: Features));
throw new ArgumentException("IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

nit: I am seeing this message will not be clear if I see it thrown. Maybe you can modify it a little to tell something like,

$"The runner metric manager is of type {_metricManager.GetType()} which expected to be of type BinaryMetricManage"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

{
var label = columnInformation.LabelColumnName;
_experiment.SetEvaluateMetric(Settings.OptimizingMetric, label);
TrialResultMonitor<MulticlassClassificationMetrics> monitor = null;

@tarekghtarekghAug 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TrialResultMonitor monitor = null;

nit: maybe better move this line down before _experiment.SetMonitor line?
This comment apply to similar places.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

}
}

throw new ArgumentException("IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

ditto.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

@tarekgh

Copy link
Copy Markdown
Member
 else

nit: you don't need the else here.


Refers to: src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:256 in 6de7519. [](commit_id = 6de7519, deletion_comment = False)

monitor.ReportFailTrial(setting, ex);
throw;
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else

else not needed here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That else will be hit when you get a training error but has a successful trial result(therefore _bestTrialResult is not null). In which case the current best result will be returned instead.

This is to avoid the case of losing all available trial results when encountering an unfatal error, like OOM or so. The more reliable way of doing that is, of course, detecting if exception from trial is fatal or not and continue training if the exception is not fatal. But in that case we need to cover all unfatal cases which is almost impossible and unnecessary. So as a step back, in order not to loss current training result, AutoMLExperiment simply 1) prints out exception and 2) return _currentBestTrial if there's any when encountering any exception. Only when there's no completed trial will AutoMLExperiment throws an exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The if block is throwing any way. so no need to have explicit else.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

OIC

Comment threadsrc/Microsoft.ML.AutoML/Tuner/EciCfoTuner.cs Outdated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added minor comments. In general the change LGTM as you explained it to me.

@LittleLittleCloud

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
LittleLittleCloud merged commit 9652e59 into dotnet:mainAug 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleLittleCloud@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use SweepablePipeline - #6285

Merged
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer
Aug 25, 2022
Merged

Use SweepablePipeline#6285
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer

Conversation

@LittleLittleCloud

@LittleLittleCloudLittleLittleCloud commented Aug 17, 2022

Copy link
Copy Markdown
Member

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Check out the spec for SweepablePipeline in #6218

SweepablePipeline is a combination of MultiModelPipeline and SweepableEstimatorPipeline, which supports a tree-like structure pipeline and support estimator-level search space using nested search space.

In another world, SweepablePipeline puts estimator candidates as part of its search space and makes it transparent to tuner. In this way, it decouples tuners from the detailed implementation of pipelines or trainers, and replacing them with Parameter and SearchSpace. The hyper-parameter optimization process, with the help of SweepablePipeline, can be simplified to the following 3 steps

  • ITuner sample parameter from search space
  • ITrialRunner train model and calculate score from parameter
  • ITuner update associated parameter with score.

Also, it provides a uniform way to create pipeline that includes multiple estimator candidates with search space.

And with this PR, the class that construct AutoML.Net Sweepable API is simplified to

  • ISweepable
    • SweepableEstimator: Estimator with search space
    • SweepablePipeline pipeline with search space

@LittleLittleCloudLittleLittleCloud changed the title [wip] Use SweepablePipelineUse SweepablePipelineAug 18, 2022
@LittleLittleCloudLittleLittleCloud changed the title Use SweepablePipeline[wip] - Use SweepablePipelineAug 18, 2022
@codecov

codecovBot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6285 (4281116) into main (8589d25) will increase coverage by 0.05%.
The diff coverage is 99.65%.

Additional details and impacted files
@@ Coverage Diff @@## main #6285 +/- ##
==========================================
+ Coverage 68.52% 68.58% +0.05% 
==========================================
Files 1170 1170 Lines 246931 247158 +227 Branches 25669 25675 +6 ==========================================
+ Hits 169220 169512 +292 + Misses 70961 70905 -56 + Partials 6750 6741 -9 
FlagCoverage Δ
Debug68.58% <99.65%> (+0.05%)⬆️
production63.01% <ø> (+0.01%)⬆️
test89.09% <99.65%> (+0.11%)⬆️

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

Impacted FilesCoverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs83.04% <98.64%> (+6.72%)⬆️
...t/Microsoft.ML.AutoML.Tests/AutoFeaturizerTests.cs92.45% <100.00%> (+0.96%)⬆️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs100.00% <100.00%> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs97.84% <100.00%> (+16.78%)⬆️
...icrosoft.ML.AutoML.Tests/SweepableExtensionTest.cs96.00% <100.00%> (+1.26%)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.38% <0.00%> (-0.15%)⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs77.94% <0.00%> (ø)
src/Microsoft.ML.Data/Data/Conversion.cs79.98% <0.00%> (+0.09%)⬆️
src/Microsoft.ML.SearchSpace/SearchSpace.cs72.01% <0.00%> (+0.45%)⬆️
... and 6 more

@LittleLittleCloudLittleLittleCloud changed the title [wip] - Use SweepablePipelineUse SweepablePipelineAug 22, 2022

public static AutoMLExperiment SetBinaryClassificationMetric(this AutoMLExperiment experiment, BinaryClassificationMetric metric, string labelColumn = "label", string predictedColumn = "PredictedLabel")
{
var metricManager = new BinaryMetricManager(metric, predictedColumn, labelColumn);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

predictedColumn, labelColumn

should we flip the order of these parameters to be consistent with the rest of APIs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

labelColumn should come before predictedColumn in order to be consistent with context.Binary.Evaluation api, I'll update BinaryMetricManager though

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved


pipeline = pipeline.Append(Context.Auto().Featurizer(trainData, columnInformation, Features));
return pipeline.Append(Context.Auto().BinaryClassification(label, useSdca: useSdca, useFastTree: useFastTree, useLgbm: useLgbm, useLbfgs: uselbfgs, useFastForest: useFastForest, featureColumnName: Features));
throw new ArgumentException("IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

nit: I am seeing this message will not be clear if I see it thrown. Maybe you can modify it a little to tell something like,

$"The runner metric manager is of type {_metricManager.GetType()} which expected to be of type BinaryMetricManage"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

{
var label = columnInformation.LabelColumnName;
_experiment.SetEvaluateMetric(Settings.OptimizingMetric, label);
TrialResultMonitor<MulticlassClassificationMetrics> monitor = null;

@tarekghtarekghAug 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TrialResultMonitor monitor = null;

nit: maybe better move this line down before _experiment.SetMonitor line?
This comment apply to similar places.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

}
}

throw new ArgumentException("IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

ditto.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

@tarekgh

Copy link
Copy Markdown
Member
 else

nit: you don't need the else here.


Refers to: src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:256 in 6de7519. [](commit_id = 6de7519, deletion_comment = False)

monitor.ReportFailTrial(setting, ex);
throw;
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else

else not needed here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That else will be hit when you get a training error but has a successful trial result(therefore _bestTrialResult is not null). In which case the current best result will be returned instead.

This is to avoid the case of losing all available trial results when encountering an unfatal error, like OOM or so. The more reliable way of doing that is, of course, detecting if exception from trial is fatal or not and continue training if the exception is not fatal. But in that case we need to cover all unfatal cases which is almost impossible and unnecessary. So as a step back, in order not to loss current training result, AutoMLExperiment simply 1) prints out exception and 2) return _currentBestTrial if there's any when encountering any exception. Only when there's no completed trial will AutoMLExperiment throws an exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The if block is throwing any way. so no need to have explicit else.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

OIC

Comment threadsrc/Microsoft.ML.AutoML/Tuner/EciCfoTuner.cs Outdated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added minor comments. In general the change LGTM as you explained it to me.

@LittleLittleCloud

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
LittleLittleCloud merged commit 9652e59 into dotnet:mainAug 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleLittleCloud@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Use SweepablePipeline - #6285

Merged
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer
Aug 25, 2022
Merged

Use SweepablePipeline#6285
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer

Conversation

@LittleLittleCloud

@LittleLittleCloudLittleLittleCloud commented Aug 17, 2022

Copy link
Copy Markdown
Member

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Check out the spec for SweepablePipeline in #6218

SweepablePipeline is a combination of MultiModelPipeline and SweepableEstimatorPipeline, which supports a tree-like structure pipeline and support estimator-level search space using nested search space.

In another world, SweepablePipeline puts estimator candidates as part of its search space and makes it transparent to tuner. In this way, it decouples tuners from the detailed implementation of pipelines or trainers, and replacing them with Parameter and SearchSpace. The hyper-parameter optimization process, with the help of SweepablePipeline, can be simplified to the following 3 steps

  • ITuner sample parameter from search space
  • ITrialRunner train model and calculate score from parameter
  • ITuner update associated parameter with score.

Also, it provides a uniform way to create pipeline that includes multiple estimator candidates with search space.

And with this PR, the class that construct AutoML.Net Sweepable API is simplified to

  • ISweepable
    • SweepableEstimator: Estimator with search space
    • SweepablePipeline pipeline with search space

@LittleLittleCloudLittleLittleCloud changed the title [wip] Use SweepablePipelineUse SweepablePipelineAug 18, 2022
@LittleLittleCloudLittleLittleCloud changed the title Use SweepablePipeline[wip] - Use SweepablePipelineAug 18, 2022
@codecov

codecovBot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6285 (4281116) into main (8589d25) will increase coverage by 0.05%.
The diff coverage is 99.65%.

Additional details and impacted files
@@ Coverage Diff @@## main #6285 +/- ##
==========================================
+ Coverage 68.52% 68.58% +0.05% 
==========================================
Files 1170 1170 Lines 246931 247158 +227 Branches 25669 25675 +6 ==========================================
+ Hits 169220 169512 +292 + Misses 70961 70905 -56 + Partials 6750 6741 -9 
FlagCoverage Δ
Debug68.58% <99.65%> (+0.05%)⬆️
production63.01% <ø> (+0.01%)⬆️
test89.09% <99.65%> (+0.11%)⬆️

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

Impacted FilesCoverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs83.04% <98.64%> (+6.72%)⬆️
...t/Microsoft.ML.AutoML.Tests/AutoFeaturizerTests.cs92.45% <100.00%> (+0.96%)⬆️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs100.00% <100.00%> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs97.84% <100.00%> (+16.78%)⬆️
...icrosoft.ML.AutoML.Tests/SweepableExtensionTest.cs96.00% <100.00%> (+1.26%)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.38% <0.00%> (-0.15%)⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs77.94% <0.00%> (ø)
src/Microsoft.ML.Data/Data/Conversion.cs79.98% <0.00%> (+0.09%)⬆️
src/Microsoft.ML.SearchSpace/SearchSpace.cs72.01% <0.00%> (+0.45%)⬆️
... and 6 more

@LittleLittleCloudLittleLittleCloud changed the title [wip] - Use SweepablePipelineUse SweepablePipelineAug 22, 2022

public static AutoMLExperiment SetBinaryClassificationMetric(this AutoMLExperiment experiment, BinaryClassificationMetric metric, string labelColumn = "label", string predictedColumn = "PredictedLabel")
{
var metricManager = new BinaryMetricManager(metric, predictedColumn, labelColumn);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

predictedColumn, labelColumn

should we flip the order of these parameters to be consistent with the rest of APIs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

labelColumn should come before predictedColumn in order to be consistent with context.Binary.Evaluation api, I'll update BinaryMetricManager though

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved


pipeline = pipeline.Append(Context.Auto().Featurizer(trainData, columnInformation, Features));
return pipeline.Append(Context.Auto().BinaryClassification(label, useSdca: useSdca, useFastTree: useFastTree, useLgbm: useLgbm, useLbfgs: uselbfgs, useFastForest: useFastForest, featureColumnName: Features));
throw new ArgumentException("IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

nit: I am seeing this message will not be clear if I see it thrown. Maybe you can modify it a little to tell something like,

$"The runner metric manager is of type {_metricManager.GetType()} which expected to be of type BinaryMetricManage"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

{
var label = columnInformation.LabelColumnName;
_experiment.SetEvaluateMetric(Settings.OptimizingMetric, label);
TrialResultMonitor<MulticlassClassificationMetrics> monitor = null;

@tarekghtarekghAug 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TrialResultMonitor monitor = null;

nit: maybe better move this line down before _experiment.SetMonitor line?
This comment apply to similar places.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

}
}

throw new ArgumentException("IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

ditto.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

@tarekgh

Copy link
Copy Markdown
Member
 else

nit: you don't need the else here.


Refers to: src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:256 in 6de7519. [](commit_id = 6de7519, deletion_comment = False)

monitor.ReportFailTrial(setting, ex);
throw;
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else

else not needed here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That else will be hit when you get a training error but has a successful trial result(therefore _bestTrialResult is not null). In which case the current best result will be returned instead.

This is to avoid the case of losing all available trial results when encountering an unfatal error, like OOM or so. The more reliable way of doing that is, of course, detecting if exception from trial is fatal or not and continue training if the exception is not fatal. But in that case we need to cover all unfatal cases which is almost impossible and unnecessary. So as a step back, in order not to loss current training result, AutoMLExperiment simply 1) prints out exception and 2) return _currentBestTrial if there's any when encountering any exception. Only when there's no completed trial will AutoMLExperiment throws an exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The if block is throwing any way. so no need to have explicit else.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

OIC

Comment threadsrc/Microsoft.ML.AutoML/Tuner/EciCfoTuner.cs Outdated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added minor comments. In general the change LGTM as you explained it to me.

@LittleLittleCloud

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
LittleLittleCloud merged commit 9652e59 into dotnet:mainAug 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleLittleCloud@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use SweepablePipeline - #6285

Merged
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer
Aug 25, 2022
Merged

Use SweepablePipeline#6285
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer

Conversation

@LittleLittleCloud

@LittleLittleCloudLittleLittleCloud commented Aug 17, 2022

Copy link
Copy Markdown
Member

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Check out the spec for SweepablePipeline in #6218

SweepablePipeline is a combination of MultiModelPipeline and SweepableEstimatorPipeline, which supports a tree-like structure pipeline and support estimator-level search space using nested search space.

In another world, SweepablePipeline puts estimator candidates as part of its search space and makes it transparent to tuner. In this way, it decouples tuners from the detailed implementation of pipelines or trainers, and replacing them with Parameter and SearchSpace. The hyper-parameter optimization process, with the help of SweepablePipeline, can be simplified to the following 3 steps

  • ITuner sample parameter from search space
  • ITrialRunner train model and calculate score from parameter
  • ITuner update associated parameter with score.

Also, it provides a uniform way to create pipeline that includes multiple estimator candidates with search space.

And with this PR, the class that construct AutoML.Net Sweepable API is simplified to

  • ISweepable
    • SweepableEstimator: Estimator with search space
    • SweepablePipeline pipeline with search space

@LittleLittleCloudLittleLittleCloud changed the title [wip] Use SweepablePipelineUse SweepablePipelineAug 18, 2022
@LittleLittleCloudLittleLittleCloud changed the title Use SweepablePipeline[wip] - Use SweepablePipelineAug 18, 2022
@codecov

codecovBot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6285 (4281116) into main (8589d25) will increase coverage by 0.05%.
The diff coverage is 99.65%.

Additional details and impacted files
@@ Coverage Diff @@## main #6285 +/- ##
==========================================
+ Coverage 68.52% 68.58% +0.05% 
==========================================
Files 1170 1170 Lines 246931 247158 +227 Branches 25669 25675 +6 ==========================================
+ Hits 169220 169512 +292 + Misses 70961 70905 -56 + Partials 6750 6741 -9 
FlagCoverage Δ
Debug68.58% <99.65%> (+0.05%)⬆️
production63.01% <ø> (+0.01%)⬆️
test89.09% <99.65%> (+0.11%)⬆️

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

Impacted FilesCoverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs83.04% <98.64%> (+6.72%)⬆️
...t/Microsoft.ML.AutoML.Tests/AutoFeaturizerTests.cs92.45% <100.00%> (+0.96%)⬆️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs100.00% <100.00%> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs97.84% <100.00%> (+16.78%)⬆️
...icrosoft.ML.AutoML.Tests/SweepableExtensionTest.cs96.00% <100.00%> (+1.26%)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.38% <0.00%> (-0.15%)⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs77.94% <0.00%> (ø)
src/Microsoft.ML.Data/Data/Conversion.cs79.98% <0.00%> (+0.09%)⬆️
src/Microsoft.ML.SearchSpace/SearchSpace.cs72.01% <0.00%> (+0.45%)⬆️
... and 6 more

@LittleLittleCloudLittleLittleCloud changed the title [wip] - Use SweepablePipelineUse SweepablePipelineAug 22, 2022

public static AutoMLExperiment SetBinaryClassificationMetric(this AutoMLExperiment experiment, BinaryClassificationMetric metric, string labelColumn = "label", string predictedColumn = "PredictedLabel")
{
var metricManager = new BinaryMetricManager(metric, predictedColumn, labelColumn);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

predictedColumn, labelColumn

should we flip the order of these parameters to be consistent with the rest of APIs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

labelColumn should come before predictedColumn in order to be consistent with context.Binary.Evaluation api, I'll update BinaryMetricManager though

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved


pipeline = pipeline.Append(Context.Auto().Featurizer(trainData, columnInformation, Features));
return pipeline.Append(Context.Auto().BinaryClassification(label, useSdca: useSdca, useFastTree: useFastTree, useLgbm: useLgbm, useLbfgs: uselbfgs, useFastForest: useFastForest, featureColumnName: Features));
throw new ArgumentException("IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

nit: I am seeing this message will not be clear if I see it thrown. Maybe you can modify it a little to tell something like,

$"The runner metric manager is of type {_metricManager.GetType()} which expected to be of type BinaryMetricManage"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

{
var label = columnInformation.LabelColumnName;
_experiment.SetEvaluateMetric(Settings.OptimizingMetric, label);
TrialResultMonitor<MulticlassClassificationMetrics> monitor = null;

@tarekghtarekghAug 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TrialResultMonitor monitor = null;

nit: maybe better move this line down before _experiment.SetMonitor line?
This comment apply to similar places.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

}
}

throw new ArgumentException("IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

ditto.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

@tarekgh

Copy link
Copy Markdown
Member
 else

nit: you don't need the else here.


Refers to: src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:256 in 6de7519. [](commit_id = 6de7519, deletion_comment = False)

monitor.ReportFailTrial(setting, ex);
throw;
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else

else not needed here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That else will be hit when you get a training error but has a successful trial result(therefore _bestTrialResult is not null). In which case the current best result will be returned instead.

This is to avoid the case of losing all available trial results when encountering an unfatal error, like OOM or so. The more reliable way of doing that is, of course, detecting if exception from trial is fatal or not and continue training if the exception is not fatal. But in that case we need to cover all unfatal cases which is almost impossible and unnecessary. So as a step back, in order not to loss current training result, AutoMLExperiment simply 1) prints out exception and 2) return _currentBestTrial if there's any when encountering any exception. Only when there's no completed trial will AutoMLExperiment throws an exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The if block is throwing any way. so no need to have explicit else.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

OIC

Comment threadsrc/Microsoft.ML.AutoML/Tuner/EciCfoTuner.cs Outdated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added minor comments. In general the change LGTM as you explained it to me.

@LittleLittleCloud

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
LittleLittleCloud merged commit 9652e59 into dotnet:mainAug 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleLittleCloud@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use SweepablePipeline - #6285

Merged
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer
Aug 25, 2022
Merged

Use SweepablePipeline#6285
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer

Conversation

@LittleLittleCloud

@LittleLittleCloudLittleLittleCloud commented Aug 17, 2022

Copy link
Copy Markdown
Member

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Check out the spec for SweepablePipeline in #6218

SweepablePipeline is a combination of MultiModelPipeline and SweepableEstimatorPipeline, which supports a tree-like structure pipeline and support estimator-level search space using nested search space.

In another world, SweepablePipeline puts estimator candidates as part of its search space and makes it transparent to tuner. In this way, it decouples tuners from the detailed implementation of pipelines or trainers, and replacing them with Parameter and SearchSpace. The hyper-parameter optimization process, with the help of SweepablePipeline, can be simplified to the following 3 steps

  • ITuner sample parameter from search space
  • ITrialRunner train model and calculate score from parameter
  • ITuner update associated parameter with score.

Also, it provides a uniform way to create pipeline that includes multiple estimator candidates with search space.

And with this PR, the class that construct AutoML.Net Sweepable API is simplified to

  • ISweepable
    • SweepableEstimator: Estimator with search space
    • SweepablePipeline pipeline with search space

@LittleLittleCloudLittleLittleCloud changed the title [wip] Use SweepablePipelineUse SweepablePipelineAug 18, 2022
@LittleLittleCloudLittleLittleCloud changed the title Use SweepablePipeline[wip] - Use SweepablePipelineAug 18, 2022
@codecov

codecovBot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6285 (4281116) into main (8589d25) will increase coverage by 0.05%.
The diff coverage is 99.65%.

Additional details and impacted files
@@ Coverage Diff @@## main #6285 +/- ##
==========================================
+ Coverage 68.52% 68.58% +0.05% 
==========================================
Files 1170 1170 Lines 246931 247158 +227 Branches 25669 25675 +6 ==========================================
+ Hits 169220 169512 +292 + Misses 70961 70905 -56 + Partials 6750 6741 -9 
FlagCoverage Δ
Debug68.58% <99.65%> (+0.05%)⬆️
production63.01% <ø> (+0.01%)⬆️
test89.09% <99.65%> (+0.11%)⬆️

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

Impacted FilesCoverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs83.04% <98.64%> (+6.72%)⬆️
...t/Microsoft.ML.AutoML.Tests/AutoFeaturizerTests.cs92.45% <100.00%> (+0.96%)⬆️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs100.00% <100.00%> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs97.84% <100.00%> (+16.78%)⬆️
...icrosoft.ML.AutoML.Tests/SweepableExtensionTest.cs96.00% <100.00%> (+1.26%)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.38% <0.00%> (-0.15%)⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs77.94% <0.00%> (ø)
src/Microsoft.ML.Data/Data/Conversion.cs79.98% <0.00%> (+0.09%)⬆️
src/Microsoft.ML.SearchSpace/SearchSpace.cs72.01% <0.00%> (+0.45%)⬆️
... and 6 more

@LittleLittleCloudLittleLittleCloud changed the title [wip] - Use SweepablePipelineUse SweepablePipelineAug 22, 2022

public static AutoMLExperiment SetBinaryClassificationMetric(this AutoMLExperiment experiment, BinaryClassificationMetric metric, string labelColumn = "label", string predictedColumn = "PredictedLabel")
{
var metricManager = new BinaryMetricManager(metric, predictedColumn, labelColumn);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

predictedColumn, labelColumn

should we flip the order of these parameters to be consistent with the rest of APIs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

labelColumn should come before predictedColumn in order to be consistent with context.Binary.Evaluation api, I'll update BinaryMetricManager though

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved


pipeline = pipeline.Append(Context.Auto().Featurizer(trainData, columnInformation, Features));
return pipeline.Append(Context.Auto().BinaryClassification(label, useSdca: useSdca, useFastTree: useFastTree, useLgbm: useLgbm, useLbfgs: uselbfgs, useFastForest: useFastForest, featureColumnName: Features));
throw new ArgumentException("IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

nit: I am seeing this message will not be clear if I see it thrown. Maybe you can modify it a little to tell something like,

$"The runner metric manager is of type {_metricManager.GetType()} which expected to be of type BinaryMetricManage"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

{
var label = columnInformation.LabelColumnName;
_experiment.SetEvaluateMetric(Settings.OptimizingMetric, label);
TrialResultMonitor<MulticlassClassificationMetrics> monitor = null;

@tarekghtarekghAug 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TrialResultMonitor monitor = null;

nit: maybe better move this line down before _experiment.SetMonitor line?
This comment apply to similar places.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

}
}

throw new ArgumentException("IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

ditto.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

@tarekgh

Copy link
Copy Markdown
Member
 else

nit: you don't need the else here.


Refers to: src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:256 in 6de7519. [](commit_id = 6de7519, deletion_comment = False)

monitor.ReportFailTrial(setting, ex);
throw;
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else

else not needed here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That else will be hit when you get a training error but has a successful trial result(therefore _bestTrialResult is not null). In which case the current best result will be returned instead.

This is to avoid the case of losing all available trial results when encountering an unfatal error, like OOM or so. The more reliable way of doing that is, of course, detecting if exception from trial is fatal or not and continue training if the exception is not fatal. But in that case we need to cover all unfatal cases which is almost impossible and unnecessary. So as a step back, in order not to loss current training result, AutoMLExperiment simply 1) prints out exception and 2) return _currentBestTrial if there's any when encountering any exception. Only when there's no completed trial will AutoMLExperiment throws an exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The if block is throwing any way. so no need to have explicit else.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

OIC

Comment threadsrc/Microsoft.ML.AutoML/Tuner/EciCfoTuner.cs Outdated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added minor comments. In general the change LGTM as you explained it to me.

@LittleLittleCloud

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
LittleLittleCloud merged commit 9652e59 into dotnet:mainAug 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleLittleCloud@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Use SweepablePipeline - #6285

Merged
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer
Aug 25, 2022
Merged

Use SweepablePipeline#6285
LittleLittleCloud merged 27 commits into
dotnet:mainfrom
LittleLittleCloud:u/xiaoyun/ISearchSpaceProposer

Conversation

@LittleLittleCloud

@LittleLittleCloudLittleLittleCloud commented Aug 17, 2022

Copy link
Copy Markdown
Member

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Check out the spec for SweepablePipeline in #6218

SweepablePipeline is a combination of MultiModelPipeline and SweepableEstimatorPipeline, which supports a tree-like structure pipeline and support estimator-level search space using nested search space.

In another world, SweepablePipeline puts estimator candidates as part of its search space and makes it transparent to tuner. In this way, it decouples tuners from the detailed implementation of pipelines or trainers, and replacing them with Parameter and SearchSpace. The hyper-parameter optimization process, with the help of SweepablePipeline, can be simplified to the following 3 steps

  • ITuner sample parameter from search space
  • ITrialRunner train model and calculate score from parameter
  • ITuner update associated parameter with score.

Also, it provides a uniform way to create pipeline that includes multiple estimator candidates with search space.

And with this PR, the class that construct AutoML.Net Sweepable API is simplified to

  • ISweepable
    • SweepableEstimator: Estimator with search space
    • SweepablePipeline pipeline with search space

@LittleLittleCloudLittleLittleCloud changed the title [wip] Use SweepablePipelineUse SweepablePipelineAug 18, 2022
@LittleLittleCloudLittleLittleCloud changed the title Use SweepablePipeline[wip] - Use SweepablePipelineAug 18, 2022
@codecov

codecovBot commented Aug 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6285 (4281116) into main (8589d25) will increase coverage by 0.05%.
The diff coverage is 99.65%.

Additional details and impacted files
@@ Coverage Diff @@## main #6285 +/- ##
==========================================
+ Coverage 68.52% 68.58% +0.05% 
==========================================
Files 1170 1170 Lines 246931 247158 +227 Branches 25669 25675 +6 ==========================================
+ Hits 169220 169512 +292 + Misses 70961 70905 -56 + Partials 6750 6741 -9 
FlagCoverage Δ
Debug68.58% <99.65%> (+0.05%)⬆️
production63.01% <ø> (+0.01%)⬆️
test89.09% <99.65%> (+0.11%)⬆️

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

Impacted FilesCoverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs83.04% <98.64%> (+6.72%)⬆️
...t/Microsoft.ML.AutoML.Tests/AutoFeaturizerTests.cs92.45% <100.00%> (+0.96%)⬆️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs100.00% <100.00%> (ø)
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs97.84% <100.00%> (+16.78%)⬆️
...icrosoft.ML.AutoML.Tests/SweepableExtensionTest.cs96.00% <100.00%> (+1.26%)⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs86.38% <0.00%> (-0.15%)⬇️
src/Microsoft.ML.Core/Data/ProgressReporter.cs77.94% <0.00%> (ø)
src/Microsoft.ML.Data/Data/Conversion.cs79.98% <0.00%> (+0.09%)⬆️
src/Microsoft.ML.SearchSpace/SearchSpace.cs72.01% <0.00%> (+0.45%)⬆️
... and 6 more

@LittleLittleCloudLittleLittleCloud changed the title [wip] - Use SweepablePipelineUse SweepablePipelineAug 22, 2022

public static AutoMLExperiment SetBinaryClassificationMetric(this AutoMLExperiment experiment, BinaryClassificationMetric metric, string labelColumn = "label", string predictedColumn = "PredictedLabel")
{
var metricManager = new BinaryMetricManager(metric, predictedColumn, labelColumn);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

predictedColumn, labelColumn

should we flip the order of these parameters to be consistent with the rest of APIs?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

labelColumn should come before predictedColumn in order to be consistent with context.Binary.Evaluation api, I'll update BinaryMetricManager though

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved


pipeline = pipeline.Append(Context.Auto().Featurizer(trainData, columnInformation, Features));
return pipeline.Append(Context.Auto().BinaryClassification(label, useSdca: useSdca, useFastTree: useFastTree, useLgbm: useLgbm, useLbfgs: uselbfgs, useFastForest: useFastForest, featureColumnName: Features));
throw new ArgumentException("IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be BinaryMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

nit: I am seeing this message will not be clear if I see it thrown. Maybe you can modify it a little to tell something like,

$"The runner metric manager is of type {_metricManager.GetType()} which expected to be of type BinaryMetricManage"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

{
var label = columnInformation.LabelColumnName;
_experiment.SetEvaluateMetric(Settings.OptimizingMetric, label);
TrialResultMonitor<MulticlassClassificationMetrics> monitor = null;

@tarekghtarekghAug 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TrialResultMonitor monitor = null;

nit: maybe better move this line down before _experiment.SetMonitor line?
This comment apply to similar places.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

}
}

throw new ArgumentException("IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"IMetricManager must be MultiMetricManager and IDatasetManager must be either TrainTestSplitDatasetManager or CrossValidationDatasetManager"

ditto.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Resolved

@tarekgh

Copy link
Copy Markdown
Member
 else

nit: you don't need the else here.


Refers to: src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:256 in 6de7519. [](commit_id = 6de7519, deletion_comment = False)

monitor.ReportFailTrial(setting, ex);
throw;
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else

else not needed here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That else will be hit when you get a training error but has a successful trial result(therefore _bestTrialResult is not null). In which case the current best result will be returned instead.

This is to avoid the case of losing all available trial results when encountering an unfatal error, like OOM or so. The more reliable way of doing that is, of course, detecting if exception from trial is fatal or not and continue training if the exception is not fatal. But in that case we need to cover all unfatal cases which is almost impossible and unnecessary. So as a step back, in order not to loss current training result, AutoMLExperiment simply 1) prints out exception and 2) return _currentBestTrial if there's any when encountering any exception. Only when there's no completed trial will AutoMLExperiment throws an exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The if block is throwing any way. so no need to have explicit else.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

OIC

Comment threadsrc/Microsoft.ML.AutoML/Tuner/EciCfoTuner.cs Outdated

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I added minor comments. In general the change LGTM as you explained it to me.

@LittleLittleCloud

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
LittleLittleCloud merged commit 9652e59 into dotnet:mainAug 25, 2022
@ghostghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleLittleCloud@tarekgh