Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updates dependencies #655

Merged
merged 19 commits into from
Apr 12, 2024
Merged

updates dependencies #655

merged 19 commits into from
Apr 12, 2024

Conversation

tharvik
Copy link
Collaborator

@tharvik tharvik commented Mar 28, 2024

currently, there is a bunch of security issues, mostly from old dependencies. this PR aims to fix that

  • build web-client with vite instead of the deprecated vue-cli
    • way faster build time, ESM hot replacement
    • add strict compile options
    • add lang="ts" to all script tags
    • commonize eslint Linting mismatch #534
  • bump all packages to latest versions
    • will reenable dependabot updates after that
    • immutable@5 is still in beta, even if npm outdated marks it as updatable
    • eslint@9 is too recent for plugins to support it yet
  • drop doc generation (outdated and unused)
  • drop dead web-client/src/components/data/preview (unused and depending on removed code)

also partially fix a race condition when receiving peers in decentralized. it would need a better synchronisation mechanism (an ACK when peer updates are done, like a real RPC) to ensure that slower nodes are indeed ready to communicate.

also rework the cypress tests to remove some uneeded files, and add a test of the training.

@tharvik tharvik self-assigned this Mar 28, 2024
@tharvik tharvik mentioned this pull request Mar 28, 2024
@tharvik tharvik force-pushed the NAN-updates-tharvik branch 2 times, most recently from 9217162 to 8fc7647 Compare April 2, 2024 15:32
@tharvik tharvik marked this pull request as ready for review April 2, 2024 15:40
@tharvik tharvik requested a review from JulienVig April 2, 2024 15:41
Copy link
Collaborator

@JulienVig JulienVig left a comment

Choose a reason for hiding this comment

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

Good work!! You managed to modify almost every single file in the repo haha. Though I don't have anything to comment on the code.

  • Can you update the Vus.js section of the Contributing guide with updated information now that we are using Vite?

  • I see you've re-written some logic in the web-client, have you played around in the web client to replace our missing tests? I've quickly trained a titanic model and the line plot doesn't seem to be updated anymore during training for example.

@tharvik tharvik force-pushed the NAN-updates-tharvik branch 3 times, most recently from 54a21b6 to 16ce26b Compare April 9, 2024 12:10
@tharvik
Copy link
Collaborator Author

tharvik commented Apr 9, 2024

soo, I driffted a bit from the original PR aim, oupsi

  • added ~workaround for "unknown signal from peer"
  • update cypress tests
  • change training to expose logs of round & epochs and avoid mutating objects
    • remove a bunch of dead/killable callbacks
    • rm now obsolete TrainingInformant
    • GraphInformant is still around as it is used by Tester/Validator; but I don't see the need for keeping track of the evolution of the accuracy, do we remove it?

@tharvik tharvik requested a review from JulienVig April 9, 2024 15:00
Copy link
Collaborator

@JulienVig JulienVig left a comment

Choose a reason for hiding this comment

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

Lots of good stuff, thanks!!

I got an error specific to silicon chips when trying to build the web-client, I copy-pasted below the stack below. I followed the advice to delete package-lock.json and node-modules and run npm i and I can now build successfully. However the package-lock.json got modified, so I'm pushing it so you can check it out and decided whether to keep the now one or revert the previous one.

Error stack
> build
> vue-tsc --build && vite build

/Users/.../disco/node_modules/rollup/dist/native.js:59
		throw new Error(
		      ^

Error: Cannot find module @rollup/rollup-darwin-arm64. npm has a bug related to optional dependencies (https://github.com/npm/cli/issues/4828). Please try `npm i` again after removing both package-lock.json and node_modules directory.
    at requireWithFriendlyError (/Users/.../disco/node_modules/rollup/dist/native.js:59:9)
    at Object.<anonymous> (/Users/.../disco/node_modules/rollup/dist/native.js:68:76)
... 3 lines matching cause stack trace ...
    at Module._load (node:internal/modules/cjs/loader:1023:12)
    at cjsLoader (node:internal/modules/esm/translators:356:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:305:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24) {
  [cause]: Error: Cannot find module '@rollup/rollup-darwin-arm64'
  Require stack:
  - /Users/.../disco/node_modules/rollup/dist/native.js
      at Module._resolveFilename (node:internal/modules/cjs/loader:1144:15)
      at Module._load (node:internal/modules/cjs/loader:985:27)
      at Module.require (node:internal/modules/cjs/loader:1235:19)
      at require (node:internal/modules/helpers:176:18)
      at requireWithFriendlyError (/Users/.../disco/node_modules/rollup/dist/native.js:41:10)
      at Object.<anonymous> (/Users/.../disco/node_modules/rollup/dist/native.js:68:76)
      at Module._compile (node:internal/modules/cjs/loader:1376:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
      at Module.load (node:internal/modules/cjs/loader:1207:32)
      at Module._load (node:internal/modules/cjs/loader:1023:12) {
    code: 'MODULE_NOT_FOUND',
    requireStack: [
      '/Users/.../disco/node_modules/rollup/dist/native.js'
    ]
  }
}

It looks like updating dependencies created a few breaking changes in the web-client...

  • During training, logs are not displayed in the "Training logs" div, but raw logs get printed in the lower left corner of the screen
    Screenshot 2024-04-10 at 11 55 03

  • After training, clicking on Test the model doesn't work and instead of a toaster, the error "Model not trained" gets also printed in the lower left corner of the page
    Screenshot 2024-04-10 at 11 51 19

If you have the time and motivation, writing test cases for these errors would be great! 😁

cli/src/cli.ts Outdated Show resolved Hide resolved
discojs/discojs-core/src/training/disco.ts Show resolved Hide resolved
@tharvik
Copy link
Collaborator Author

tharvik commented Apr 11, 2024

I got an error specific to silicon chips when trying to build the web-client
However the package-lock.json got modified, so I'm pushing it so you can check it out and decided whether to keep the now one or revert the previous one.

yeah, it's a faster moving space now that dependencies are actual again. vue and friends are moving fast and we're only enforcing major versions.
btw, in between your remarks and me updating the PR, it changed again 🚀

  • During training, logs are not displayed in the "Training logs" div, but raw logs get printed in the lower left corner of the screen

ho, I forgot to actually load a theme in the Toaster! I don't think we really need messages at all (the toaster itself is alreay quite fancy, there are better way to show that's models are training such as via the graphs we have).
but for now, I forwarded the logger output in there, that makes it a bit more fun to watch.
and a test that messages are shown.

  • After training, clicking on Test the model doesn't work and instead of a toaster, the error "Model not trained" gets also printed in the lower left corner of the page

ho, it seems that the model is not stored in memory automatically anymore. last commit should fix it.

If you have the time and motivation, writing test cases for these errors would be great! 😁

héhé, and regressions tests added. it's actually super fast to write some with vitest's components testing. we have no more excuses!

another point: after discussion with Klavdiia, it seems that I forgot to reimplement the calling of onBatch{Begin,End}, so no communication (or saving of the models) was done anymore! it wasn't catched because the server tests weren't shuffeling the datasets and the same initial model was used.

@tharvik tharvik requested a review from JulienVig April 11, 2024 10:38
Copy link
Collaborator

@JulienVig JulienVig left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I noticed an error when predicting after training a model and I pushed a fix myself, there's a high chance this bug is my fault...

@tharvik tharvik merged commit d1e2be5 into develop Apr 12, 2024
23 checks passed
@tharvik tharvik deleted the NAN-updates-tharvik branch April 12, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants