Repository navigation
Conversation
55697bc to
e489b7f
Compare
e489b7f to
0e054da
Compare
0e054da to
e76d17c
Compare
e76d17c to
a7b402f
Compare
ee165f3 to
f335779
Compare
f335779 to
f6cc2e0
Compare
0112c7a to
6da8cc1
Compare
|
I've also refactored the |
be145af to
bdc2a99
Compare
JulienVig
left a comment
There was a problem hiding this comment.
Thanks for the great work! I mostly have nitpicking comments but I'm requesting changes because the CI is currently flaky. I just merged #1259 which should fix the error. You can request the review again once you rebased and see that the CI passes consistently
| const browsers = process.env.DISCO_E2E_BROWSERS?.split(",") ?? [ | ||
| "chrome", | ||
| "chrome", | ||
| "firefox", |
There was a problem hiding this comment.
| "firefox", | |
| "edge", |
Could you swap one of the firefoxes by edge? I realized that in a hospital use case, most people would probably use the default microsoft browser. Since it's chromium based it shouldn't change much but let's include since it doesn't cost anything
| import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process"; | ||
| import { createInterface } from "node:readline"; | ||
|
|
||
| const browsers = process.env.DISCO_E2E_BROWSERS?.split(",") ?? [ |
There was a problem hiding this comment.
Is DISCO_E2E_BROWSERS ever used? If not let's remove it and we can add it back when we actually need it. Otherwise can you document it?
Also this split is error prone if whitespaces are used for ex: "chrome, firefox", I think it should fail loudly if the env contains whitespace
| process.exitCode = 1; | ||
| }); | ||
|
|
||
| const results = browsers.map((browser, index) => { |
There was a problem hiding this comment.
The 4 browsers are started sequentially but MNIST by default only waits for 3 peers to start aggregation, this can leave the last browser hanging. This is maybe the reason why the CI failed for your other PR.
You could overwrite the min number of participants for MNIST to 4
| for (const child of children) { | ||
| if (child.pid === undefined || child.exitCode !== null) continue; | ||
| try { | ||
| process.kill(-child.pid, "SIGTERM"); |
There was a problem hiding this comment.
| process.kill(-child.pid, "SIGTERM"); | |
| process.kill(-child.pid, "SIGTERM"); // minus sign means kill both cypress and its browser |
| cy.contains("button", "Start training").click(); | ||
| assertNoErrorToast(); | ||
|
|
||
| const expectedParticipants = Cypress.env("numberOfParticipants") ?? 4; |
There was a problem hiding this comment.
Cypress.env is deprecated. Use cy.env or Cypress.expose instead
| cy.contains("button", "group").click(); | ||
| cy.task<string[]>("readdir", "../datasets/CIFAR10").then((files) => | ||
| cy | ||
| .contains("h4", "Group label: 0") |
There was a problem hiding this comment.
| .contains("h4", "Group label: 0") | |
| .contains("h4", "Group label: 0") // dummy labels for now |
| cy.visit("/"); | ||
| cy.contains("a", "Start training").click(); | ||
| cy.get(".driver-popover-close-btn").click(); | ||
| cy.get("#mnist", { timeout: 30_000 }) | ||
| .contains("button", "participate") | ||
| .click(); |
There was a problem hiding this comment.
I think you could extend goToTaskOverview to accept an optional task ID and go to task if specified
| cy.contains("h6", "Collaborative model sharing") | ||
| .next({ timeout: 270_000 }) | ||
| .should("have.text", "10"); | ||
| cy.contains("Training successfully completed", { timeout: 270_000 }); |
There was a problem hiding this comment.
| cy.contains("h6", "Collaborative model sharing") | |
| .next({ timeout: 270_000 }) | |
| .should("have.text", "10"); | |
| cy.contains("Training successfully completed", { timeout: 270_000 }); | |
| cy.contains("h6", "Collaborative model sharing") | |
| .next() | |
| .should("have.text", "10"); | |
| cy.contains("Training successfully completed"); |
I think that once we reached epoch 20 there's no need to add such long timeouts
| const expectedParticipants = Cypress.env("numberOfParticipants") ?? 4; | ||
|
|
||
| cy.contains("h6", "number of participants") | ||
| .next({ timeout: 270_000 }) |
There was a problem hiding this comment.
Do we actually need to wait that long before the 3 participants are connected?
| child.once("exit", (code, signal) => { | ||
| const passed = code === 0; | ||
| if (!passed) { | ||
| console.error(`[${label}] exited with ${code ?? signal}`); | ||
| stopChildren(); | ||
| } |
There was a problem hiding this comment.
Could you save and print which browser failed first? (For example by saving the label in variable that you only set if undefined). This would be helpful for understanding what went wrong
What & why
Adds end-to-end webapp coverage for decentralized browser-to-browser training, as requested in #1196 and the review of #1230. Four real browser participants (two Chrome and two Firefox) train the default MNIST task through the webapp against a live Disco server over WebRTC. The run exercises Chrome–Chrome, Firefox–Firefox, and Chrome–Firefox model exchange.
Technical plan
Keep the test with the other webapp training suites. Start the standard built Disco server and a Vite webapp, then run four Cypress browser processes on a shared Xvfb display in CI. Assert that all participants complete 20 epochs and 10 model-sharing rounds.
Deviations from plan
None. This replaces the Node participant prototype in #1232 with browser-to-browser collaboration.
Todo
None.