Cancellation in Image Classification (fixes #4632) - #4650

Merged
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation
Jan 17, 2020
Merged

Cancellation in Image Classification (fixes #4632)#4650
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation

Conversation

@antoniovs1029

@antoniovs1029antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

Adds support for cancellation to the Image Classification trainer in a similar manner as done in #3062 (and other PRs) by adding cancellation checkpoints to the train method.

I've tested it by running the sample related to this trainer. Since the other PR's that included checkpoints for cancellation don't include unit tests, I also didn't include any in here.

Fixes#4632 .

@antoniovs1029
antoniovs1029 requested a review from a team as a code ownerJanuary 13, 2020 23:09
@antoniovs1029

antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

I don't know if I should also add a .CheckAlive() chekpoint inside the CacheFeaturizedImagesToDisk method of Image Classification Trainer, as that method can take a couple of minutes, but once the method is over, the trainer will anyway end up hitting the checkpoint I've already added in TrainAndEvaluateClassificationLayer.

Also, if anyone has other opinions as to where to put more checkpoints, please, let me know!


for (int epoch = 0; epoch < epochs; epoch += 1)
{
Host.CheckAlive();

@codemzscodemzsJan 14, 2020

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.

Host.CheckAlive(); [](start = 20, length = 18)

I would just put the check in this loop and in the CreateFeaturizedCacheFile. Please also report numbers in perf differences before and after. Please remove CheckAlive from everywhere else as its not very significant and only pollutes the code. You also need to call TryCleanupTemporaryWorkspace for a graceful termination. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a new method "CheckAlive" to the ImageClassification trainer, with a try...catch to call TryCleanupTemporaryWorkspace when it's needed.

Also changed the places where I added the checkpoints.

I will see how to get the perf difference now. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So I ran the ImageClassificationBench.TrainResnetV250 benchmark, with and without the changes of this PR, and they both behaved in pretty much the same way.

Without the changes this was the summary output of the benchmark:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 41.55 s | 5.580 s | 0.3058 s | - |

And with the changes, the summary was:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 40.10 s | 2.723 s | 0.1493 s | - |

So on average the version with the changes was reported to ran faster.

In any case, the CheckAlive() method is simply doing if-statements evaluations, so I don't think it can introduce meaningful performance difference (given that image classification training is a task expected to take a considerable amount of time anyway). #Closed

@antoniovs1029antoniovs1029Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So, as suggested online by @codemzs I have reran the benchmarks, but using the CIFAR-10 dataset.

Without the changes introduced in the PR the summary is as follows:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 79.29 m | 4.850 m | 0.2658 m | - |

With the changes:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|--------:|-------------:|
TrainResnetV250 | 78.82 m | 21.71 m | 1.190 m | - |

So, again, my understanding is that there's some variability in the time it takes to train this model (and that's why the benchmark with the changes ran a little bit faster), and the introduction of the CheckAlive() method doesn't really have an impact on the performance of this. #Closed

@codemzs

Copy link
Copy Markdown
Member

CacheFeaturizedImagesToDisk can take significant time, we must add there.


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

@codemzscodemzs 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.

:shipit:

@antoniovs1029
antoniovs1029 merged commit 6210c38 into dotnet:masterJan 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 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.

image classification needs cancel

2 participants

@antoniovs1029@codemzs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e 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

Cancellation in Image Classification (fixes #4632) - #4650

Merged
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation
Jan 17, 2020
Merged

Cancellation in Image Classification (fixes #4632)#4650
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation

Conversation

@antoniovs1029

@antoniovs1029antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

Adds support for cancellation to the Image Classification trainer in a similar manner as done in #3062 (and other PRs) by adding cancellation checkpoints to the train method.

I've tested it by running the sample related to this trainer. Since the other PR's that included checkpoints for cancellation don't include unit tests, I also didn't include any in here.

Fixes#4632 .

@antoniovs1029
antoniovs1029 requested a review from a team as a code ownerJanuary 13, 2020 23:09
@antoniovs1029

antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

I don't know if I should also add a .CheckAlive() chekpoint inside the CacheFeaturizedImagesToDisk method of Image Classification Trainer, as that method can take a couple of minutes, but once the method is over, the trainer will anyway end up hitting the checkpoint I've already added in TrainAndEvaluateClassificationLayer.

Also, if anyone has other opinions as to where to put more checkpoints, please, let me know!


for (int epoch = 0; epoch < epochs; epoch += 1)
{
Host.CheckAlive();

@codemzscodemzsJan 14, 2020

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.

Host.CheckAlive(); [](start = 20, length = 18)

I would just put the check in this loop and in the CreateFeaturizedCacheFile. Please also report numbers in perf differences before and after. Please remove CheckAlive from everywhere else as its not very significant and only pollutes the code. You also need to call TryCleanupTemporaryWorkspace for a graceful termination. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a new method "CheckAlive" to the ImageClassification trainer, with a try...catch to call TryCleanupTemporaryWorkspace when it's needed.

Also changed the places where I added the checkpoints.

I will see how to get the perf difference now. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So I ran the ImageClassificationBench.TrainResnetV250 benchmark, with and without the changes of this PR, and they both behaved in pretty much the same way.

Without the changes this was the summary output of the benchmark:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 41.55 s | 5.580 s | 0.3058 s | - |

And with the changes, the summary was:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 40.10 s | 2.723 s | 0.1493 s | - |

So on average the version with the changes was reported to ran faster.

In any case, the CheckAlive() method is simply doing if-statements evaluations, so I don't think it can introduce meaningful performance difference (given that image classification training is a task expected to take a considerable amount of time anyway). #Closed

@antoniovs1029antoniovs1029Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So, as suggested online by @codemzs I have reran the benchmarks, but using the CIFAR-10 dataset.

Without the changes introduced in the PR the summary is as follows:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 79.29 m | 4.850 m | 0.2658 m | - |

With the changes:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|--------:|-------------:|
TrainResnetV250 | 78.82 m | 21.71 m | 1.190 m | - |

So, again, my understanding is that there's some variability in the time it takes to train this model (and that's why the benchmark with the changes ran a little bit faster), and the introduction of the CheckAlive() method doesn't really have an impact on the performance of this. #Closed

@codemzs

Copy link
Copy Markdown
Member

CacheFeaturizedImagesToDisk can take significant time, we must add there.


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

@codemzscodemzs 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.

:shipit:

@antoniovs1029
antoniovs1029 merged commit 6210c38 into dotnet:masterJan 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 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.

image classification needs cancel

2 participants

@antoniovs1029@codemzs
, '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

Cancellation in Image Classification (fixes #4632) - #4650

Merged
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation
Jan 17, 2020
Merged

Cancellation in Image Classification (fixes #4632)#4650
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation

Conversation

@antoniovs1029

@antoniovs1029antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

Adds support for cancellation to the Image Classification trainer in a similar manner as done in #3062 (and other PRs) by adding cancellation checkpoints to the train method.

I've tested it by running the sample related to this trainer. Since the other PR's that included checkpoints for cancellation don't include unit tests, I also didn't include any in here.

Fixes#4632 .

@antoniovs1029
antoniovs1029 requested a review from a team as a code ownerJanuary 13, 2020 23:09
@antoniovs1029

antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

I don't know if I should also add a .CheckAlive() chekpoint inside the CacheFeaturizedImagesToDisk method of Image Classification Trainer, as that method can take a couple of minutes, but once the method is over, the trainer will anyway end up hitting the checkpoint I've already added in TrainAndEvaluateClassificationLayer.

Also, if anyone has other opinions as to where to put more checkpoints, please, let me know!


for (int epoch = 0; epoch < epochs; epoch += 1)
{
Host.CheckAlive();

@codemzscodemzsJan 14, 2020

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.

Host.CheckAlive(); [](start = 20, length = 18)

I would just put the check in this loop and in the CreateFeaturizedCacheFile. Please also report numbers in perf differences before and after. Please remove CheckAlive from everywhere else as its not very significant and only pollutes the code. You also need to call TryCleanupTemporaryWorkspace for a graceful termination. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a new method "CheckAlive" to the ImageClassification trainer, with a try...catch to call TryCleanupTemporaryWorkspace when it's needed.

Also changed the places where I added the checkpoints.

I will see how to get the perf difference now. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So I ran the ImageClassificationBench.TrainResnetV250 benchmark, with and without the changes of this PR, and they both behaved in pretty much the same way.

Without the changes this was the summary output of the benchmark:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 41.55 s | 5.580 s | 0.3058 s | - |

And with the changes, the summary was:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 40.10 s | 2.723 s | 0.1493 s | - |

So on average the version with the changes was reported to ran faster.

In any case, the CheckAlive() method is simply doing if-statements evaluations, so I don't think it can introduce meaningful performance difference (given that image classification training is a task expected to take a considerable amount of time anyway). #Closed

@antoniovs1029antoniovs1029Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So, as suggested online by @codemzs I have reran the benchmarks, but using the CIFAR-10 dataset.

Without the changes introduced in the PR the summary is as follows:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 79.29 m | 4.850 m | 0.2658 m | - |

With the changes:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|--------:|-------------:|
TrainResnetV250 | 78.82 m | 21.71 m | 1.190 m | - |

So, again, my understanding is that there's some variability in the time it takes to train this model (and that's why the benchmark with the changes ran a little bit faster), and the introduction of the CheckAlive() method doesn't really have an impact on the performance of this. #Closed

@codemzs

Copy link
Copy Markdown
Member

CacheFeaturizedImagesToDisk can take significant time, we must add there.


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

@codemzscodemzs 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.

:shipit:

@antoniovs1029
antoniovs1029 merged commit 6210c38 into dotnet:masterJan 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 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.

image classification needs cancel

2 participants

@antoniovs1029@codemzs
, '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 \u003e 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

Cancellation in Image Classification (fixes #4632) - #4650

Merged
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation
Jan 17, 2020
Merged

Cancellation in Image Classification (fixes #4632)#4650
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation

Conversation

@antoniovs1029

@antoniovs1029antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

Adds support for cancellation to the Image Classification trainer in a similar manner as done in #3062 (and other PRs) by adding cancellation checkpoints to the train method.

I've tested it by running the sample related to this trainer. Since the other PR's that included checkpoints for cancellation don't include unit tests, I also didn't include any in here.

Fixes#4632 .

@antoniovs1029
antoniovs1029 requested a review from a team as a code ownerJanuary 13, 2020 23:09
@antoniovs1029

antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

I don't know if I should also add a .CheckAlive() chekpoint inside the CacheFeaturizedImagesToDisk method of Image Classification Trainer, as that method can take a couple of minutes, but once the method is over, the trainer will anyway end up hitting the checkpoint I've already added in TrainAndEvaluateClassificationLayer.

Also, if anyone has other opinions as to where to put more checkpoints, please, let me know!


for (int epoch = 0; epoch < epochs; epoch += 1)
{
Host.CheckAlive();

@codemzscodemzsJan 14, 2020

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.

Host.CheckAlive(); [](start = 20, length = 18)

I would just put the check in this loop and in the CreateFeaturizedCacheFile. Please also report numbers in perf differences before and after. Please remove CheckAlive from everywhere else as its not very significant and only pollutes the code. You also need to call TryCleanupTemporaryWorkspace for a graceful termination. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a new method "CheckAlive" to the ImageClassification trainer, with a try...catch to call TryCleanupTemporaryWorkspace when it's needed.

Also changed the places where I added the checkpoints.

I will see how to get the perf difference now. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So I ran the ImageClassificationBench.TrainResnetV250 benchmark, with and without the changes of this PR, and they both behaved in pretty much the same way.

Without the changes this was the summary output of the benchmark:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 41.55 s | 5.580 s | 0.3058 s | - |

And with the changes, the summary was:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 40.10 s | 2.723 s | 0.1493 s | - |

So on average the version with the changes was reported to ran faster.

In any case, the CheckAlive() method is simply doing if-statements evaluations, so I don't think it can introduce meaningful performance difference (given that image classification training is a task expected to take a considerable amount of time anyway). #Closed

@antoniovs1029antoniovs1029Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So, as suggested online by @codemzs I have reran the benchmarks, but using the CIFAR-10 dataset.

Without the changes introduced in the PR the summary is as follows:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 79.29 m | 4.850 m | 0.2658 m | - |

With the changes:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|--------:|-------------:|
TrainResnetV250 | 78.82 m | 21.71 m | 1.190 m | - |

So, again, my understanding is that there's some variability in the time it takes to train this model (and that's why the benchmark with the changes ran a little bit faster), and the introduction of the CheckAlive() method doesn't really have an impact on the performance of this. #Closed

@codemzs

Copy link
Copy Markdown
Member

CacheFeaturizedImagesToDisk can take significant time, we must add there.


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

@codemzscodemzs 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.

:shipit:

@antoniovs1029
antoniovs1029 merged commit 6210c38 into dotnet:masterJan 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 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.

image classification needs cancel

2 participants

@antoniovs1029@codemzs
, '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

Cancellation in Image Classification (fixes #4632) - #4650

Merged
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation
Jan 17, 2020
Merged

Cancellation in Image Classification (fixes #4632)#4650
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation

Conversation

@antoniovs1029

@antoniovs1029antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

Adds support for cancellation to the Image Classification trainer in a similar manner as done in #3062 (and other PRs) by adding cancellation checkpoints to the train method.

I've tested it by running the sample related to this trainer. Since the other PR's that included checkpoints for cancellation don't include unit tests, I also didn't include any in here.

Fixes#4632 .

@antoniovs1029
antoniovs1029 requested a review from a team as a code ownerJanuary 13, 2020 23:09
@antoniovs1029

antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

I don't know if I should also add a .CheckAlive() chekpoint inside the CacheFeaturizedImagesToDisk method of Image Classification Trainer, as that method can take a couple of minutes, but once the method is over, the trainer will anyway end up hitting the checkpoint I've already added in TrainAndEvaluateClassificationLayer.

Also, if anyone has other opinions as to where to put more checkpoints, please, let me know!


for (int epoch = 0; epoch < epochs; epoch += 1)
{
Host.CheckAlive();

@codemzscodemzsJan 14, 2020

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.

Host.CheckAlive(); [](start = 20, length = 18)

I would just put the check in this loop and in the CreateFeaturizedCacheFile. Please also report numbers in perf differences before and after. Please remove CheckAlive from everywhere else as its not very significant and only pollutes the code. You also need to call TryCleanupTemporaryWorkspace for a graceful termination. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a new method "CheckAlive" to the ImageClassification trainer, with a try...catch to call TryCleanupTemporaryWorkspace when it's needed.

Also changed the places where I added the checkpoints.

I will see how to get the perf difference now. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So I ran the ImageClassificationBench.TrainResnetV250 benchmark, with and without the changes of this PR, and they both behaved in pretty much the same way.

Without the changes this was the summary output of the benchmark:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 41.55 s | 5.580 s | 0.3058 s | - |

And with the changes, the summary was:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 40.10 s | 2.723 s | 0.1493 s | - |

So on average the version with the changes was reported to ran faster.

In any case, the CheckAlive() method is simply doing if-statements evaluations, so I don't think it can introduce meaningful performance difference (given that image classification training is a task expected to take a considerable amount of time anyway). #Closed

@antoniovs1029antoniovs1029Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So, as suggested online by @codemzs I have reran the benchmarks, but using the CIFAR-10 dataset.

Without the changes introduced in the PR the summary is as follows:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 79.29 m | 4.850 m | 0.2658 m | - |

With the changes:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|--------:|-------------:|
TrainResnetV250 | 78.82 m | 21.71 m | 1.190 m | - |

So, again, my understanding is that there's some variability in the time it takes to train this model (and that's why the benchmark with the changes ran a little bit faster), and the introduction of the CheckAlive() method doesn't really have an impact on the performance of this. #Closed

@codemzs

Copy link
Copy Markdown
Member

CacheFeaturizedImagesToDisk can take significant time, we must add there.


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

@codemzscodemzs 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.

:shipit:

@antoniovs1029
antoniovs1029 merged commit 6210c38 into dotnet:masterJan 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 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.

image classification needs cancel

2 participants

@antoniovs1029@codemzs
, '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

Cancellation in Image Classification (fixes #4632) - #4650

Merged
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation
Jan 17, 2020
Merged

Cancellation in Image Classification (fixes #4632)#4650
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation

Conversation

@antoniovs1029

@antoniovs1029antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

Adds support for cancellation to the Image Classification trainer in a similar manner as done in #3062 (and other PRs) by adding cancellation checkpoints to the train method.

I've tested it by running the sample related to this trainer. Since the other PR's that included checkpoints for cancellation don't include unit tests, I also didn't include any in here.

Fixes#4632 .

@antoniovs1029
antoniovs1029 requested a review from a team as a code ownerJanuary 13, 2020 23:09
@antoniovs1029

antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

I don't know if I should also add a .CheckAlive() chekpoint inside the CacheFeaturizedImagesToDisk method of Image Classification Trainer, as that method can take a couple of minutes, but once the method is over, the trainer will anyway end up hitting the checkpoint I've already added in TrainAndEvaluateClassificationLayer.

Also, if anyone has other opinions as to where to put more checkpoints, please, let me know!


for (int epoch = 0; epoch < epochs; epoch += 1)
{
Host.CheckAlive();

@codemzscodemzsJan 14, 2020

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.

Host.CheckAlive(); [](start = 20, length = 18)

I would just put the check in this loop and in the CreateFeaturizedCacheFile. Please also report numbers in perf differences before and after. Please remove CheckAlive from everywhere else as its not very significant and only pollutes the code. You also need to call TryCleanupTemporaryWorkspace for a graceful termination. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a new method "CheckAlive" to the ImageClassification trainer, with a try...catch to call TryCleanupTemporaryWorkspace when it's needed.

Also changed the places where I added the checkpoints.

I will see how to get the perf difference now. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So I ran the ImageClassificationBench.TrainResnetV250 benchmark, with and without the changes of this PR, and they both behaved in pretty much the same way.

Without the changes this was the summary output of the benchmark:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 41.55 s | 5.580 s | 0.3058 s | - |

And with the changes, the summary was:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 40.10 s | 2.723 s | 0.1493 s | - |

So on average the version with the changes was reported to ran faster.

In any case, the CheckAlive() method is simply doing if-statements evaluations, so I don't think it can introduce meaningful performance difference (given that image classification training is a task expected to take a considerable amount of time anyway). #Closed

@antoniovs1029antoniovs1029Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So, as suggested online by @codemzs I have reran the benchmarks, but using the CIFAR-10 dataset.

Without the changes introduced in the PR the summary is as follows:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 79.29 m | 4.850 m | 0.2658 m | - |

With the changes:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|--------:|-------------:|
TrainResnetV250 | 78.82 m | 21.71 m | 1.190 m | - |

So, again, my understanding is that there's some variability in the time it takes to train this model (and that's why the benchmark with the changes ran a little bit faster), and the introduction of the CheckAlive() method doesn't really have an impact on the performance of this. #Closed

@codemzs

Copy link
Copy Markdown
Member

CacheFeaturizedImagesToDisk can take significant time, we must add there.


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

@codemzscodemzs 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.

:shipit:

@antoniovs1029
antoniovs1029 merged commit 6210c38 into dotnet:masterJan 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 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.

image classification needs cancel

2 participants

@antoniovs1029@codemzs
, '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

Cancellation in Image Classification (fixes #4632) - #4650

Merged
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation
Jan 17, 2020
Merged

Cancellation in Image Classification (fixes #4632)#4650
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation

Conversation

@antoniovs1029

@antoniovs1029antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

Adds support for cancellation to the Image Classification trainer in a similar manner as done in #3062 (and other PRs) by adding cancellation checkpoints to the train method.

I've tested it by running the sample related to this trainer. Since the other PR's that included checkpoints for cancellation don't include unit tests, I also didn't include any in here.

Fixes#4632 .

@antoniovs1029
antoniovs1029 requested a review from a team as a code ownerJanuary 13, 2020 23:09
@antoniovs1029

antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

I don't know if I should also add a .CheckAlive() chekpoint inside the CacheFeaturizedImagesToDisk method of Image Classification Trainer, as that method can take a couple of minutes, but once the method is over, the trainer will anyway end up hitting the checkpoint I've already added in TrainAndEvaluateClassificationLayer.

Also, if anyone has other opinions as to where to put more checkpoints, please, let me know!


for (int epoch = 0; epoch < epochs; epoch += 1)
{
Host.CheckAlive();

@codemzscodemzsJan 14, 2020

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.

Host.CheckAlive(); [](start = 20, length = 18)

I would just put the check in this loop and in the CreateFeaturizedCacheFile. Please also report numbers in perf differences before and after. Please remove CheckAlive from everywhere else as its not very significant and only pollutes the code. You also need to call TryCleanupTemporaryWorkspace for a graceful termination. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a new method "CheckAlive" to the ImageClassification trainer, with a try...catch to call TryCleanupTemporaryWorkspace when it's needed.

Also changed the places where I added the checkpoints.

I will see how to get the perf difference now. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So I ran the ImageClassificationBench.TrainResnetV250 benchmark, with and without the changes of this PR, and they both behaved in pretty much the same way.

Without the changes this was the summary output of the benchmark:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 41.55 s | 5.580 s | 0.3058 s | - |

And with the changes, the summary was:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 40.10 s | 2.723 s | 0.1493 s | - |

So on average the version with the changes was reported to ran faster.

In any case, the CheckAlive() method is simply doing if-statements evaluations, so I don't think it can introduce meaningful performance difference (given that image classification training is a task expected to take a considerable amount of time anyway). #Closed

@antoniovs1029antoniovs1029Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So, as suggested online by @codemzs I have reran the benchmarks, but using the CIFAR-10 dataset.

Without the changes introduced in the PR the summary is as follows:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 79.29 m | 4.850 m | 0.2658 m | - |

With the changes:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|--------:|-------------:|
TrainResnetV250 | 78.82 m | 21.71 m | 1.190 m | - |

So, again, my understanding is that there's some variability in the time it takes to train this model (and that's why the benchmark with the changes ran a little bit faster), and the introduction of the CheckAlive() method doesn't really have an impact on the performance of this. #Closed

@codemzs

Copy link
Copy Markdown
Member

CacheFeaturizedImagesToDisk can take significant time, we must add there.


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

@codemzscodemzs 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.

:shipit:

@antoniovs1029
antoniovs1029 merged commit 6210c38 into dotnet:masterJan 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 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.

image classification needs cancel

2 participants

@antoniovs1029@codemzs
, '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

Cancellation in Image Classification (fixes #4632) - #4650

Merged
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation
Jan 17, 2020
Merged

Cancellation in Image Classification (fixes #4632)#4650
antoniovs1029 merged 11 commits into
dotnet:masterfrom
antoniovs1029:is18cancelation

Conversation

@antoniovs1029

@antoniovs1029antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
Contributor

Adds support for cancellation to the Image Classification trainer in a similar manner as done in #3062 (and other PRs) by adding cancellation checkpoints to the train method.

I've tested it by running the sample related to this trainer. Since the other PR's that included checkpoints for cancellation don't include unit tests, I also didn't include any in here.

Fixes#4632 .

@antoniovs1029
antoniovs1029 requested a review from a team as a code ownerJanuary 13, 2020 23:09
@antoniovs1029

antoniovs1029 commented Jan 13, 2020

Copy link
Copy Markdown
ContributorAuthor

I don't know if I should also add a .CheckAlive() chekpoint inside the CacheFeaturizedImagesToDisk method of Image Classification Trainer, as that method can take a couple of minutes, but once the method is over, the trainer will anyway end up hitting the checkpoint I've already added in TrainAndEvaluateClassificationLayer.

Also, if anyone has other opinions as to where to put more checkpoints, please, let me know!


for (int epoch = 0; epoch < epochs; epoch += 1)
{
Host.CheckAlive();

@codemzscodemzsJan 14, 2020

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.

Host.CheckAlive(); [](start = 20, length = 18)

I would just put the check in this loop and in the CreateFeaturizedCacheFile. Please also report numbers in perf differences before and after. Please remove CheckAlive from everywhere else as its not very significant and only pollutes the code. You also need to call TryCleanupTemporaryWorkspace for a graceful termination. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a new method "CheckAlive" to the ImageClassification trainer, with a try...catch to call TryCleanupTemporaryWorkspace when it's needed.

Also changed the places where I added the checkpoints.

I will see how to get the perf difference now. #Closed

@antoniovs1029antoniovs1029Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So I ran the ImageClassificationBench.TrainResnetV250 benchmark, with and without the changes of this PR, and they both behaved in pretty much the same way.

Without the changes this was the summary output of the benchmark:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 41.55 s | 5.580 s | 0.3058 s | - |

And with the changes, the summary was:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 40.10 s | 2.723 s | 0.1493 s | - |

So on average the version with the changes was reported to ran faster.

In any case, the CheckAlive() method is simply doing if-statements evaluations, so I don't think it can introduce meaningful performance difference (given that image classification training is a task expected to take a considerable amount of time anyway). #Closed

@antoniovs1029antoniovs1029Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So, as suggested online by @codemzs I have reran the benchmarks, but using the CIFAR-10 dataset.

Without the changes introduced in the PR the summary is as follows:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|---------:|-------------:|
TrainResnetV250 | 79.29 m | 4.850 m | 0.2658 m | - |

With the changes:

 Method | Mean | Error | StdDev | Extra Metric |
---------------- |--------:|--------:|--------:|-------------:|
TrainResnetV250 | 78.82 m | 21.71 m | 1.190 m | - |

So, again, my understanding is that there's some variability in the time it takes to train this model (and that's why the benchmark with the changes ran a little bit faster), and the introduction of the CheckAlive() method doesn't really have an impact on the performance of this. #Closed

@codemzs

Copy link
Copy Markdown
Member

CacheFeaturizedImagesToDisk can take significant time, we must add there.


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

@codemzscodemzs 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.

:shipit:

@antoniovs1029
antoniovs1029 merged commit 6210c38 into dotnet:masterJan 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Mar 19, 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.

image classification needs cancel

2 participants

@antoniovs1029@codemzs