Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ITrialResultManager for continue training in AutoML #6335

Merged

Conversation

LittleLittleCloud
Copy link
Contributor

@LittleLittleCloud LittleLittleCloud commented Sep 20, 2022

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.

Resolved issue

#5736

What's ITrialResultManager

ITrialResultManager is a component used by AutoMLExperiment to save and load TrialResult. The default implementation for ITrialResultManager is CsvTrialResultManager, which saves all trial results in csv format

What's ITrialResultManager used for

Currently, it's used by some tuners (CostFrugalTuner, for example) to recover from previous training, suppose trialResult csv is provided or is available.

How to use it

ITrialResultManager is not directly exposed to public users. User can use SetCheckpoint(string folder) path instead. If a folder is provided via SetCheckpoint, AutoMLExperiment will create that folder if not exists, and configures CsvTrialResultManager to save trial results to folder/trialResults.csv.

@LittleLittleCloud LittleLittleCloud changed the title Add ITrialResultManager Add ITrialResultManager for continue training in AutoML Sep 22, 2022
@@ -12,13 +12,13 @@ namespace Microsoft.ML.AutoML
public interface IMetricManager
{
bool IsMaximize { get; }

string MetricName { get; }
Copy link
Member

Choose a reason for hiding this comment

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

string

I assume there is no app compat concern extending this interface. Extending interfaces on .NET Framework is a breaking change.

"loss",
"durationInMilliseconds",
"peakCpu",
"peakMemoryInMegaByte"
Copy link
Member

Choose a reason for hiding this comment

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

nit: please align this block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

{
if (!File.Exists(filePath))
{
return new TrialResult[0];
Copy link
Member

Choose a reason for hiding this comment

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

new TrialResult[0];

Array.Empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

@@ -56,6 +54,11 @@ public Flow2(SearchSpace.SearchSpace searchSpace, Parameter initValue = null, bo
{
_step = _stepUpperBound;
}

if (rng != null)
Copy link
Member

Choose a reason for hiding this comment

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

(rng != null)

nit: you don't need this condition I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

{
// initialize eci with the estimated cost and always start from pipeline which has lowest cost.
_eci = _pipelineSchemas.ToDictionary(kv => kv, kv => GetEstimatedCostForPipeline(kv, _sweepablePipeline));
_initialized = true;
Copy link
Member

Choose a reason for hiding this comment

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

_initialized

nit: move this line after setting the schema. just in case to be safer for multi-threading scenarios if it is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

}

public void Update(TrialResult result, string schema)
{
_initialized = true;
Copy link
Member

Choose a reason for hiding this comment

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

_initialized

should you set this after finishing the update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

}

// make sure hash code is greater than 0
return (int)(hash >> 1);
Copy link
Member

Choose a reason for hiding this comment

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

Is it picked up from Java? :-)

You can simplify that like
https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Tuple.cs,69

without using checked blocks at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's cool, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Added minor comments and questions. LGTM otherwise.

@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #6335 (b9d7a41) into main (632c373) will decrease coverage by 0.07%.
The diff coverage is 39.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6335      +/-   ##
==========================================
- Coverage   68.58%   68.51%   -0.08%     
==========================================
  Files        1171     1165       -6     
  Lines      247964   246142    -1822     
  Branches    25738    25555     -183     
==========================================
- Hits       170076   168635    -1441     
+ Misses      71129    70834     -295     
+ Partials     6759     6673      -86     
Flag Coverage Δ
Debug 68.51% <39.47%> (-0.08%) ⬇️
production 62.93% <100.00%> (-0.04%) ⬇️
test 89.00% <24.59%> (-0.12%) ⬇️

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

Impacted Files Coverage Δ
test/Microsoft.ML.AutoML.Tests/AutoFitTests.cs 84.94% <ø> (+2.46%) ⬆️
...icrosoft.ML.AutoML.Tests/TrialResultManagerTest.cs 0.00% <0.00%> (ø)
src/Microsoft.ML.SearchSpace/SearchSpace.cs 73.36% <100.00%> (+1.34%) ⬆️
src/Microsoft.ML.SearchSpace/Tuner/RandomTuner.cs 60.00% <100.00%> (-21.82%) ⬇️
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs 96.23% <100.00%> (+3.84%) ⬆️
.../Microsoft.ML.AutoML.Tests/CostFrugalTunerTests.cs 97.20% <100.00%> (+1.30%) ⬆️
.../Microsoft.ML.SearchSpace.Tests/SearchSpaceTest.cs 100.00% <100.00%> (ø)
src/Microsoft.ML.Maml/MAML.cs 24.36% <0.00%> (-2.54%) ⬇️
src/Microsoft.Data.Analysis/DataFrame.IO.cs 79.88% <0.00%> (-1.28%) ⬇️
... and 19 more

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

disable SweepablePipeline_AutoFit_UCI_Adult_CrossValidation_10_Test
@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud
Copy link
Contributor Author

/azp --help

@azure-pipelines
Copy link

Command '--help' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@LittleLittleCloud
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@LittleLittleCloud LittleLittleCloud merged commit cba0162 into dotnet:main Oct 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 11, 2022
Sign up for free to 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