Uh oh!
There was an error while loading. Please reload this page.
reimplement binary experiment using AutoMLExperiment - #6246
Conversation
| public sealed class BinaryClassificationExperiment : ExperimentBase<BinaryClassificationMetrics, BinaryExperimentSettings> | ||
| { | ||
| private readonly AutoMLExperiment _experiment; | ||
| private const string Features = "__Features__"; |
There was a problem hiding this comment.
Output column name for Concatenating all features
| /// Depending on the size of your data, the AutoML experiment could take a long time to execute. | ||
| /// </remarks> | ||
| public ExperimentResult<TMetrics> Execute(IDataView trainData, string labelColumnName = DefaultColumnNames.Label, | ||
| public virtual ExperimentResult<TMetrics> Execute(IDataView trainData, string labelColumnName = DefaultColumnNames.Label, |
There was a problem hiding this comment.
add virtual keyword so it can be overrided
| { EstimatorType.SdcaMaximumEntropyMulti, 1.129 }, | ||
| { EstimatorType.SdcaLogisticRegressionOva, 3.16 }, | ||
| { EstimatorType.LightGbmMulti, 4.765 }, | ||
| { EstimatorType.SdcaMaximumEntropyMulti, 10.129 }, |
There was a problem hiding this comment.
Sdca converges slower than I expected. The original cost for sdca is calculated with subsampling prerposer so it's much smaller than what it costs on the entire dataset.
Considering that it's very, very unlikely for sdca to be the ace trainer among all available trainers, which means the time spent on Sdca is in most case a waste, it's acceptable to increase the initial cost of Sdca so it will have the least priority in the entire trainer selection.
| context = new MLContext(1); | ||
| var modelTrainTest = context.Auto() | ||
| .CreateBinaryClassificationExperiment(0) | ||
| .CreateBinaryClassificationExperiment(10) |
There was a problem hiding this comment.
It's necessary to increase the maximize training time from 0 because the logic for max time limitation is a bit different comparing the old and new implementations. In the old implementations, binary classification experiment will wait for the final trial to complete even the time budget is used up. In the new implementation however, binary classification experiment will try to cancel all trials running on that context.
There was a problem hiding this comment.
I think we should change this behavior back to the old behavior. I don't think cancelling all running trials is a good idea. If I don't know how long my data will take to train and I set it for 2 hours and nothing finished by then, I would probably be pretty annoyed to have it cancel part of the way through.
Or maybe what would be better would be to have both ways possible. 1 way to force cancel everything, and 1 way to cancel but let it finish gracefully. Until we are able to have that though, I don't think we should forcefully cancel all running pipelines especially if non have finished yet.
There was a problem hiding this comment.
I would still vote for forcing canceling trial because the only situation for force canceling running trial ruins user experience is when there's no trial completed and user waits a few hours for nothing. That situation can be avoided by making the first trial end quickly, by starting from a small model or starting from a portion of dataset. ModelBuilder uses those techniques and it goes pretty well. The new AutoML also starts from a small model, and I'm working on a PR for starting from a portion of dataset when necessary.
Gracefully canceling, however, has more annoying situations that are easier to encounter. For example, when time is up and AutoML is training a large model, it would be really frustrating to wait another few minutes for the final trial to complete, especially in notebook running cell
There was a problem hiding this comment.
We can probably do a mixed way: force canceling when there's completed trial, and wait for the first trial to complete if there's no completed trials.
There was a problem hiding this comment.
I think a mixed way is a good idea for now. But I don't think we should just cancel when nothing has finished. Is that a complex change for you to add?
Codecov Report
@@ Coverage Diff @@## main #6246 +/- ##
=======================================
Coverage 68.44% 68.45% =======================================
Files 1141 1141 Lines 244790 244820 +30 Branches 25405 25405 =======================================
+ Hits 167551 167580 +29 - Misses 70599 70602 +3 + Partials 6640 6638 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| else | ||
| { | ||
| // TODO |
There was a problem hiding this comment.
I agree. I wouldn't abort the whole thing for a single failure (though there still should be some threshold. Not a blocker for this PR, just agreeing with your comment.
| /// <summary> | ||
| /// TrialResult with Binary Classification Metrics | ||
| /// </summary> | ||
| internal class BinaryClassificationTrialResult : TrialResult |
There was a problem hiding this comment.
Do we not want the user to have access to this class?
There was a problem hiding this comment.
Nope, we just want user to have access to TrialResult only. The difference between BinaryClassificationTrialResult and TrialResult is the first one contains all binary classification metrics, which is for backward compatibility and avoid changing the behavoir of old AutoML only. So I'd rather BinaryClassificationTrialResult not expose to public
There was a problem hiding this comment.
Will they not need the extra information? If not then sounds good.
There was a problem hiding this comment.
No they shouldn't need those extra metric informations. I don't understand why the old AutoML.Net API returns all metrics together even though the tuner only optimizes based on one of them. It's probably just a mis-design.
michaelgsharp
left a comment
There was a problem hiding this comment.
Mostly questions, but the issue with the trail cancellation does need to be resolved.
michaelgsharp
commented
Jul 15, 2022
@LittleLittleCloud looks like this test, AutoFitWithPresplittedData, should also be changed to a LightGbmFact instead of just fact. |
LittleLittleCloud
commented
Jul 15, 2022
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
We are excited to review your PR.
So we can do the best job, please check:
Fixes #nnnnin your description to cause GitHub to automatically close the issue(s) when your PR is merged.This PR reimplements binary classification experiment using AutoMLExperiment, while keeping all the API unchanged so the existing documents don't need to be updated.