Draft: GPT2 training on MCQ med data#1111
Conversation
mina5rovic
changed the title
fd4b676 to
3014cf5
Compare
…k for model to contxt 512
JulienVig
left a comment
JulienVig
left a comment
There was a problem hiding this comment.
1st part of my review, I'll finish at a different time, feel free to start addressing these comments
| roundIterations?: number; | ||
| batchSize: number; | ||
| validationSplit: number; | ||
| validationFrequency?: number; | ||
| datasetPath?: string; | ||
| validationDatasetPath?: string; |
There was a problem hiding this comment.
Can you document how all the validation parameters interact with each others? For example what happens if you specify both validationSplit and validationDatasetPath?
| debugProcessMemory( | ||
| `[${shortenId(this.ownId)}] round ${round} before encode`, | ||
| ); | ||
| const payload = await serialization.weights.encode(payloadToServer); | ||
| debugProcessMemory( | ||
| `[${shortenId(this.ownId)}] round ${round} after encode`, |
There was a problem hiding this comment.
is it useful to log the before after?
There was a problem hiding this comment.
It was useful when debugging, can remove it now.
| id: NodeID; | ||
| waitForMoreParticipants: boolean; | ||
| payload: serialization.Encoded; | ||
| payload?: serialization.Encoded | null; |
There was a problem hiding this comment.
Because payload is null only at round 0, when no global weights exist yet. The server sets it to undefined, but MessagePack decodes undefined as null, so the interface must allow null. It was one of the silent error, took some time to catch.
| ? this.#runRounds(dataset, validationDataset) | ||
| : this.#runIterationRounds(dataset, validationDataset); |
There was a problem hiding this comment.
I think these method names are not explicit enough, what about #runRoundsByEpoch and #runRoundsByIteration? Can you add documentation for each method to clearly differentiate their intended use and behaviors?
There was a problem hiding this comment.
Same comment for runRound and runIterationRound
| const model = this.model as unknown as IterationTrainableTextModel; | ||
| if (typeof model.trainNextBatches !== "function") | ||
| throw new Error("model does not support iteration-based training"); | ||
|
|
There was a problem hiding this comment.
I find this part really inconvenient because it's not generic, it only addresses on particular case. Here's an idea to work around this, let me know what you think
- Implement a new dataset method that splits the dataset after a specified number of steps and returns 1) one dataset with only the next specified number of batches and 2) the remainder of the dataset after these batches. e.g., const [nextBatches, datasetRemainder] = dataset.subset(maxBatchCount)
- From runIterationRounds, I think this allows you to do reuse the existing #runRound, e.g. this.#runRound(nextBatches, roundValidationDataset)
- With this method I don't think you need to deal with all the iterators
Let me know what you think and whether this is feasible
Co-authored-by: Julien Vignoud <33122365+JulienVig@users.noreply.github.com>
Co-authored-by: Julien Vignoud <33122365+JulienVig@users.noreply.github.com>
Co-authored-by: Julien Vignoud <33122365+JulienVig@users.noreply.github.com>
Co-authored-by: Julien Vignoud <33122365+JulienVig@users.noreply.github.com>
2 participants
No description provided.