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

Update doc according npm workspace #636

Merged
merged 30 commits into from
Feb 28, 2024
Merged

Update doc according npm workspace #636

merged 30 commits into from
Feb 28, 2024

Conversation

JulienVig
Copy link
Collaborator

@JulienVig JulienVig commented Feb 22, 2024

I reorganized the documentation with 3 different types of DISCO users in mind. What they should be able to do are concentric subsets:

  1. non-technical users → want to use the UI to train and create their own custom tasks without coding
  2. technical users → additionally code in Node.js to train and create their own custom tasks with discojs-node and discojs-server
  3. contributors → additionally need to be able to build, test, modify and learn in-depth information

As such, I organized the doc by following these successive levels:

  1. README.md for everyone and especially non-technical users
  2. DEV.md for technical users and contributors, it now contains instructions for the different ways to use DISCO
  3. CONTRIBUTING.md for contributors, with build, test, coding conventions and in-depth information needed to modify the code base.

Additionally:

  • Removed redundant module readme (discojs, disco-core/node/web, web-client) as instructions are now centralized in DEV.md and CONTRIBUTING.md
  • Merged ONBOARDING.md into CONTRIBUTING.md and deleted the former. I didn't find the difference between the two files self-explanatory enough and contributors have to go through both of them anyway so I merged the two into one file.
  • Created a new example for adding a custom task to the server in docs/node_example, which I renamed docs/examples with the objective of adding further examples in the future.

Out of scope: #639 Adding custom tasks is currently very convoluted and needs a rework to make it easier for external users. Information on how to add custom tasks is not clear and not accessible enough for any of the three user categories I mentioned.

@JulienVig JulienVig linked an issue Feb 22, 2024 that may be closed by this pull request
12 tasks
@JulienVig JulienVig self-assigned this Feb 22, 2024
@JulienVig JulienVig added documentation Improvements or additions to documentation cli Related to the CLI module labels Feb 22, 2024
@JulienVig JulienVig marked this pull request as ready for review February 28, 2024 12:49
@JulienVig JulienVig requested a review from tharvik February 28, 2024 12:49
Copy link
Collaborator

@tharvik tharvik left a comment

Choose a reason for hiding this comment

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

superbe, thanks for the tiding up 🥇

comments are not blocking, rather "nice to have"

DEV.md Outdated
There are multiple ways to use and interact with DISCO, depending on your objective:

- A non-technical user that wants to train models in a distributed manner without coding would want to use DISCO through the `web-client`. To do so, starting a local `server` instance is also needed as a backend to the `web-client`. Similarly, a contributor aiming to implement new UI features would certainly want to run the same setup.
- A technical user may find it more flexible to use DISCO from a Node.js script, which gives users a finer control over the process. The `discojs-node` module is tailored to be used in Node.js scripts and allows to load data, start a server and run distributed machine learning training tasks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

starting a server is in the server package only, but indeed discojs-node can help build a server

cli/package.json Outdated Show resolved Hide resolved
cli/src/args.ts Outdated Show resolved Hide resolved
Comment on lines +81 to +82
npm -w web-client start # from the root folder
npm start # from the web-client folder
Copy link
Collaborator

Choose a reason for hiding this comment

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

hum, sometimes commands are from the root folder, sometimes from the local dir. we need to keep to only one style of command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case I'm giving two alternative ways of starting the web-client depending on the cwd. I'm trying to always use -w but I'm sometimes wary that some readers may have missed how -w worked

Copy link
Collaborator

Choose a reason for hiding this comment

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

your call :)

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/examples/README.md Outdated Show resolved Hide resolved
docs/examples/tsconfig.json Outdated Show resolved Hide resolved
docs/examples/package.json Outdated Show resolved Hide resolved
docs/examples/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we add linting support here at all? if feel a bit out of scope. the current npm run lint doesn't work (but can made to work if merge the root eslintrc with the local one)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that would be nice, how can I do it? I'm not what you mean by "merge tje root eslintrc"

Copy link
Collaborator

Choose a reason for hiding this comment

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

eslint uses a cascading config organisation, meaning that it aggregates all eslintrc files start from cwd and bubbling up the parent chain.
in the current case, it takes docs/examples/.eslintrc.json & .eslintrc.js, making the example non-local. you can copy the one from root in docs/examples and change its env to use node instead of mocha.

JulienVig and others added 7 commits February 28, 2024 17:05
Co-authored-by: Valérian Rousset <tharvik@users.noreply.github.com>
Co-authored-by: Valérian Rousset <tharvik@users.noreply.github.com>
Co-authored-by: Valérian Rousset <tharvik@users.noreply.github.com>
Co-authored-by: Valérian Rousset <tharvik@users.noreply.github.com>
Co-authored-by: Valérian Rousset <tharvik@users.noreply.github.com>
Co-authored-by: Valérian Rousset <tharvik@users.noreply.github.com>
@JulienVig JulienVig merged commit 5717c4b into develop Feb 28, 2024
@JulienVig JulienVig deleted the 635-update-doc-julien branch February 28, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the CLI module documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update module installation doc The CLI isn't working
2 participants