Conversation
302f156 to
67b00eb
Compare
8875ceb to
04c5fda
Compare
JulienVig
left a comment
JulienVig
left a comment
There was a problem hiding this comment.
Thanks for your work! Looks overall good to me, I left a few minor comments that you can address before merging
| cy.contains("button", "next").click(); | ||
| cy.contains("button", "locally").click(); | ||
| cy.contains("button", "Start training").click(); | ||
| cy.contains("Training successfully completed", { | ||
| timeout: trainingTimeout, | ||
| }).should("be.visible"); | ||
| cy.contains("h6", "epochs") | ||
| .next() | ||
| .should("have.text", `${epochs} / ${epochs}`); | ||
| cy.contains("button", "Start training").should("be.visible"); | ||
| cy.contains("button", "next").click(); | ||
| cy.contains("button", "save model").click(); | ||
| cy.contains(`The trained ${title} model has been saved.`, { | ||
| timeout: 30_000, | ||
| }).should("be.visible"); | ||
| } |
There was a problem hiding this comment.
Can you also check that no error toaster are being shown as in training.cy.ts? They can sometimes be displayed even when the training proceeds without crashing
cy.get(".v-toast__item--error", { timeout: 1_000 }).should("not.exist");I've seen them occur after clicking start training, at the end of training or after clicking save model.
| wait-on: http://localhost:1351 | ||
| browser: electron |
There was a problem hiding this comment.
| wait-on: http://localhost:1351 | |
| browser: electron |
Electron is already the default isn't it?
Cypress already waits for the webapp to boot, no need to manually wait for it
There was a problem hiding this comment.
This test is a duplicate of the one in training.cy.ts, I like the folder structure you made in this PR so can you move tests out of training.cy.ts into titanic.cy.ts and lus_covid.ts and delete duplicate ones?
|
|
||
| cy.contains("button", "test model").click(); | ||
| cy.url().should("include", "/evaluate"); | ||
| cy.contains("Titanic Prediction"); |
There was a problem hiding this comment.
"Titanic Prediction" is found in the success toaster but this is already tested in trainLocallyAndSave
| cy.contains("Titanic Prediction"); | |
| cy.contains("Model Testing"); |
| infos.props("rounds").last()?.epochs.last()?.training.accuracy, | ||
| ).toBeGreaterThan(0); | ||
|
|
||
| await vi.waitFor(() => expect(infos.props("isTraining")).toBe(false)); |
There was a problem hiding this comment.
| await vi.waitFor(() => expect(infos.props("isTraining")).toBe(false)); | |
| await vi.waitFor(() => expect(infos.props("isTraining")).toBe(false), { timeout: 5_000 }); |
The default 1s timeout is maybe a bit tight
| it("completes local LUS COVID training and saves the model", () => { | ||
| setupServerWith( | ||
| withTrainingConfig(defaultTasks.lusCovid, { | ||
| epochs: 4, |
There was a problem hiding this comment.
Use a variable for the epoch numer to keep it consistent with the argument of trainLocallyAndSave
| .first() | ||
| .selectFile("../datasets/titanic_train.csv"); | ||
|
|
||
| trainLocallyAndSave("Titanic Prediction", 10); |
There was a problem hiding this comment.
For the epoch number you can directly use the default task epoch parameter
This branch has not been deployed
2 participants
What & why
This PR adds dedicated browser end-to-end coverage for local training and runs the new specs independently in CI. Titanic uses the full bundled CSV and its default task configuration. LUS COVID selects all bundled images and uses the real model, preprocessing, batch size, and validation setup, with a CI-specific five-epoch workload so the test remains bounded.
Technical plan
The general E2E job excludes the dedicated local-training specs, while a matrix runs the Titanic and LUS scenarios independently. Each scenario mocks only task/model retrieval, selects local files through the UI, completes training, and saves the resulting model.
Deviations from plan
The issue calls for coverage of each data type; this PR does not include Wikitext because a parallel GPT-2 browser workload is too resource-intensive. The default 50-epoch LUS task exceeded five minutes on the GitHub Electron runner, so CI uses five complete epochs over the full bundled image dataset.
Todo
Add representative text-training coverage when a practical browser workload is established.