-
Notifications
You must be signed in to change notification settings - Fork 149
TrainingLoop: refactor progress printer and add CSVLogger #668
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,70 @@ | ||||||
| import Foundation | ||||||
| import ModelSupport | ||||||
|
|
||||||
| /// A callback-based handler for logging the statistics to CSV file. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice doc comment. English nit:
Suggested change
I would consider not leading with “callback-based.” In fact, "logging" and "CSV File" are kind of implied by the name. So the best description would explain what's being logged. “Statistics” is good, but what kind fo statistics? Training statistics, maybe? Maybe this should be thought of as “a log file” rather than “a logger?”
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the doc comment. Any reason why this is LogFile? (Logger makes sense to me since its a logger not a file) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This thing can only be constructed to log to a path in the filesystem, and when we invoke its one public method, If I see this code, I know exactly what's happening. // No argument label needed, arguably, because when you construct a thing called “File” with
// a string, the string is obviously a path.
LogFile eventLog(s)
eventLog.append(blah, blah, blah)With |
||||||
| public class CSVLogger { | ||||||
| public var path: String | ||||||
|
xihui-wu marked this conversation as resolved.
|
||||||
|
|
||||||
| let foundationFS: FoundationFileSystem | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| let foundationFile: FoundationFile | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| /// Create an instance that log statistics during the training loop. | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| public init(withPath path: String = "run/log.csv") { | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| self.path = path | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems very unlikely to me that we actually need to store
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. Removed foundationFile and kept the path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure you want to open and close the file for every line logged, though. (I am presuming that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How did you “resolve” this comment? |
||||||
| self.foundationFS = FoundationFileSystem() | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| self.foundationFile = FoundationFile(path: path) | ||||||
| } | ||||||
|
|
||||||
| /// The callback used to hook into the TrainingLoop for logging statistics. | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| /// | ||||||
| /// - Parameters: | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| /// - loop: The TrainingLoop where an event has occurred. | ||||||
| /// - event: The training or validation event that this callback is responding to. | ||||||
| public func log<L: TrainingLoopProtocol>(_ loop: inout L, event: TrainingLoopEvent) throws { | ||||||
|
xihui-wu marked this conversation as resolved.
|
||||||
| switch event { | ||||||
| case .batchEnd: | ||||||
| guard let epochIndex = loop.epochIndex, let epochCount = loop.epochCount, | ||||||
| let batchIndex = loop.batchIndex, let batchCount = loop.batchCount | ||||||
| else { | ||||||
| return | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would any of these things be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Merged stats into the same guard statements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, first, I would rather see something like because it describes the situation at a semantic level rather than at the level of what some code did. (also writing "No-Op" adds nothing to what is already very obvious from the code) But that said, it seems very unlikely that the comment I'd like to see is true of any but the last property. All the others refer to values that have nothing to do with logging. So I want to know what causes It's a big design flaw in trainingLoop that it has so many optionals, and that makes this task more difficult, but I believe that's not the code you're working on(?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These variables were originally designed to be optionals to store temporary data in protocol: https://github.com/tensorflow/swift-models/blob/master/TrainingLoop/TrainingLoop.swift#L76 The generic TrainingLoop that implements the protocol does set all these optionals. My guess on why it was designed so is that it allows other TrainingLoops not setting them. So the point on the comment is NOT "if stats logging doesn't request it then these properties will be nil", but "if these properties are nil No-op on the CSVLogger". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then you should delete the comment. The code very clearly says that all by itself, so the comment explains nothing. I don't know if you're missing the point I'm trying to make, or you just disagree with it, but I'm doing this code review 100% for your benefit as a programmer. If the review process is blocking your progress, please feel free to just commit the changes, and decide separately about whether you want the feedback I'm giving you here. If you do, we can continue to discuss it. |
||||||
| } | ||||||
|
|
||||||
| guard let stats = loop.lastStatsLog else { | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| if !FileManager.default.fileExists(atPath: path) { | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| try foundationFS.createDirectoryIfMissing(at: String(path[..<path.lastIndex(of: "/")!])) | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
xihui-wu marked this conversation as resolved.
Outdated
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| try writeHeader(stats: stats) | ||||||
| } | ||||||
| try writeDataRow( | ||||||
|
xihui-wu marked this conversation as resolved.
|
||||||
| epoch: "\(epochIndex + 1)/\(epochCount)", | ||||||
| batch: "\(batchIndex + 1)/\(batchCount)", | ||||||
| stats: stats) | ||||||
| default: | ||||||
| return | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func writeHeader(stats: [(String, Float)]) throws { | ||||||
| let head: String = (["epoch", "batch"] + stats.map { $0.0 }).joined(separator: ", ") | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
xihui-wu marked this conversation as resolved.
Outdated
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| do { | ||||||
| try head.write(toFile: path, atomically: true, encoding: .utf8) | ||||||
| } catch { | ||||||
| print("Unexpected error in writing header line: \(error).") | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| throw error | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func writeDataRow(epoch: String, batch: String, stats: [(String, Float)]) throws { | ||||||
| let dataRow: Data = ( | ||||||
|
dabrahams marked this conversation as resolved.
Outdated
|
||||||
| "\n" + ([epoch, batch] + stats.map { String($0.1) }).joined(separator: ", ") | ||||||
|
xihui-wu marked this conversation as resolved.
Outdated
|
||||||
| ).data(using: .utf8)! | ||||||
| do { | ||||||
| try foundationFile.append(dataRow) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it important to do this atomically if there are multiple writers to the same log file?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does FileHandle support atomically write ? (Also, we won't expect multiple files written at same time I think) |
||||||
| } catch { | ||||||
| print("Unexpected error in writing data row: \(error).") | ||||||
| throw error | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| // Copyright 2020 The TensorFlow Authors. All Rights Reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| import Foundation | ||
|
|
||
| let progressBarLength = 30 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. length in what unit? I suppose it's probably characters. Being a top-level declaration this should have a doc comment, and that would be a perfect place to put the answer. Is this the number of |
||
|
|
||
| /// A callback-based handler for printing the training or validation progress. | ||
| public class ProgressPrinter { | ||
|
|
||
| /// Create an instance that prints progress during the training loop. | ||
| /// The progress contains a dynamic progress bar followed by statistics of metrics. | ||
| public init() { | ||
| } | ||
|
|
||
| /// The callback used to hook into the TrainingLoop for printing progress. | ||
| /// | ||
| /// An example of the progress would be: | ||
| /// Epoch 1/12 | ||
| /// 468/468 [==============================] - loss: 0.4819 - accuracy: 0.8513 | ||
| /// 79/79 [==============================] - loss: 0.1520 - accuracy: 0.9521 | ||
| /// | ||
| /// - Parameters: | ||
| /// - loop: The TrainingLoop where an event has occurred. | ||
| /// - event: The training or validation event that this callback is responding to. | ||
| public func print<L: TrainingLoopProtocol>(_ loop: inout L, event: TrainingLoopEvent) throws { | ||
| switch event { | ||
| case .epochStart: | ||
| guard let epochIndex = loop.epochIndex, let epochCount = loop.epochCount else { | ||
| return | ||
| } | ||
|
|
||
| Swift.print("Epoch \(epochIndex + 1)/\(epochCount)") | ||
| case .batchEnd: | ||
| guard let batchIndex = loop.batchIndex, let batchCount = loop.batchCount else { | ||
| return | ||
| } | ||
|
|
||
| let progressBar = formatProgressBar( | ||
| progress: Float(batchIndex + 1) / Float(batchCount), length: progressBarLength) | ||
| var stats: String = "" | ||
| if let lastStatsLog = loop.lastStatsLog { | ||
| stats = formatStats(lastStatsLog) | ||
| } | ||
|
|
||
| Swift.print( | ||
| "\r\(batchIndex + 1)/\(batchCount) \(progressBar)\(stats)", | ||
| terminator: "" | ||
| ) | ||
| fflush(stdout) | ||
| case .epochEnd: | ||
| Swift.print("") | ||
| case .validationStart: | ||
| Swift.print("") | ||
| default: | ||
| return | ||
| } | ||
| } | ||
|
|
||
| func formatProgressBar(progress: Float, length: Int) -> String { | ||
| let progressSteps = Int(round(Float(length) * progress)) | ||
| let leading = String(repeating: "=", count: progressSteps) | ||
| let separator: String | ||
| let trailing: String | ||
| if progressSteps < progressBarLength { | ||
| separator = ">" | ||
| trailing = String(repeating: ".", count: progressBarLength - progressSteps - 1) | ||
| } else { | ||
| separator = "" | ||
| trailing = "" | ||
| } | ||
| return "[\(leading)\(separator)\(trailing)]" | ||
| } | ||
|
|
||
| func formatStats(_ stats: [(String, Float)]) -> String { | ||
| var result = "" | ||
| for stat in stats { | ||
| result += " - \(stat.0): \(String(format: "%.4f", stat.1))" | ||
| } | ||
| return result | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.