reworks packages names

[tharvik]

Based on remarks from @JulienVig, WDYT?

The name on NPM doesn't match the ones in the repo, that's confusing, and some paths are nested unnecessarily. Proposed list of renaming, from old name to new name (both FS and NPM)

• discojs/discojs-core -> discojs
• discojs/discojs-node -> discojs-node
  • thin layer over discojs, might not be needed if merged inside server/benchmark
• discojs/discojs-web -> discojs-web # merged in webapp
• server -> server (not published to NPM)
• cli -> cli (not published)
  • reduces the scope of the tool, from what I gathered, there is no need for a full CLI moar CLI in the future
• web-client -> webapp (not published to NPM)

[martinjaggi]

excellent

want to rename first or first prioritize the bugfixing of the current main functionality issues listed by julien?

for cli: could this also help how disco can be integrated into existing apps? headless mode, would be nice to describe a bit how to do that (in which case it would be more than just benchmarking). but if that's not cli then sure fine to call it benchmarking

[JulienVig]

Looks good! I have few concerns though:

1. If discojs-node is merged with the server then it gets confusing to import Client-side material from discojs-server.
2. A user would never import discojs itself which may be confusing? Is it a good idea to use discojs for something only contributors will encounter? I don't have other ideas though so I'm fine going with it anyway I saw that tfjs does the same thing between pure javascript (@tensorflow/tfjs) and node (@tensorflow/tfjs-node) so I think it's a good idea to follow the pattern and rename discojs-core as discojs
3. Once there is a discojs-server users may be wondering why discojs-web isn't the webapp. So I think we should indeed merge discojs-web and webapp. Unless we want to let contributors create other UI's, which I don't think has been an important point so far.

I think the renaming and other refactors should wait until the current PRs are merged and bugs are fixed.

It would be nice to have a headless mode but so far the CLI was only used as benchmark so it'd be a new feature. Maybe we can leave it as CLI for now since it's quite a clear name already.

[tharvik]

want to rename first or first prioritize the bugfixing of the current main functionality issues listed by julien?

bug fixing is the most important for now, this issue is for after the fixes (and some tests also).

for cli: could this also help how disco can be integrated into existing apps? headless mode, would be nice to describe a bit how to do that (in which case it would be more than just benchmarking). but if that's not cli then sure fine to call it benchmarking
[...]
It would be nice to have a headless mode but so far the CLI was only used as benchmark so it'd be a new feature. Maybe we can leave it as CLI for now since it's quite a clear name already.

keeping it as cli for now, we can discuss later on how/what to add to it (same functionnality as the webapp? only basics as benchmark/train/predict?)

1. If discojs-node is merged with the server then it gets confusing to import Client-side material from discojs-server.

I don't follow you there, which client-side material are you talking of?
I hesitated quite a bit to actually rename it as a discojs-$extension but there was a disco-server NPM packages so I got pushed into following that. not publishing it on NPM and keeping it as server is making a bit more sense to me but I'm not sure of how people actually want to try it out (docker comes to mind to where server should run).

3. Once there is a discojs-server users may be wondering why discojs-web isn't the webapp. So I think we should indeed merge discojs-web and webapp. Unless we want to let contributors create other UI's, which I don't think has been an important point so far.

good point, let's merge theses. we can unmerge it should the need arise (it's only two small files anyway)

[JulienVig]

I don't follow you there, which client-side material are you talking of?

For example discojs-node currently exports some objects that are not server-related, such as the Disco object which, despite its confusing name, represents a client and exposes a .fitDataset method. Importing such an object from a module named discojs-server seems counter-intuitive to me.
In other words, a user can rely on the current discojs-node for things that have nothing to do with running a server (such as data preprocessing, training a local model etc) so merging discojs-node into the server may create confusion.

I think keeping it separate would make sense. Or what about merging discojs-node and server and naming the union discojs-node (rather than discojs-server)? As such users can import client and server instances from the same import named discojs-node which I think makes intuitive sense since both rely on Node.js. Would it make sense to include the server implementation in the Disco.js library?

Indeed according to the doc the server is deployed via docker.

[JulienVig]

So I think we should indeed merge discojs-web and webapp. Unless we want to let contributors create other UI's, which I don't think has been an important point so far.

let's merge theses. we can unmerge it should the need arise (it's only two small files anyway)

I just read the NextJS PR. Merging discojs-web into webapp means that we are not planning to integrate the NextJS frontend.

[peacefulotter]

I think keeping it separate would make sense. Or what about merging discojs-node and server and naming the union discojs-node (rather than discojs-server)? As such users can import client and server instances from the same import named discojs-node which I think makes intuitive sense since both rely on Node.js. Would it make sense to include the server implementation in the Disco.js library?

If I may, I think this is a good idea as it appears discojs-node needs the server anyways.
However, I would keep discojs-web separate from the web app since there is a pretty clear distinction between them: the web app uses discojs-web and is not part of it.
Overall, here is a proposition:

.
├─ cli/
├─ disco/
│   ├── core/
│   ├── node/
│   │   │   ├── plugins/ # Contains discojs/discojs-node, still keeping a slight separation from the server since it clearly extends code from core/
│   │   │   └── server/ # Contains server/
│   └── web/
│   │   │   └── plugins/ # Contains discojs/discojs-web, named plugins to have the same format as for node
#   │   │   └── webapp/ # If decided to be merged with discojs-web
├─ docs/
└─ webapp/

[tharvik]

In other words, a user can rely on the current discojs-node for things that have nothing to do with running a server (such as data preprocessing, training a local model etc) so merging discojs-node into the server may create confusion.
I think keeping it separate would make sense.

right, it'll be nice to allow users to run disco locally (ie w/ node and w/o a server). keeping the discojs-node separated will probably ease that. keeping it separated then.

Or what about merging discojs-node and server and naming the union discojs-node (rather than discojs-server)? As such users can import client and server instances from the same import named discojs-node which I think makes intuitive sense since both rely on Node.js. Would it make sense to include the server implementation in the Disco.js library?

hum, I don't think so: the server should be dumb, really only exposing discojs calls via HTTP, not much more (a thin axios/bun layer will work), most of the logic should go into discojs. discojs-node will only provide a few node-specifics helpers in this case.

If I may, I think this is a good idea as it appears discojs-node needs the server anyways.

indeed, it currently does, but it shouldn't, we need to have a tree of deps not a full graph.

However, I would keep discojs-web separate from the web app since there is a pretty clear distinction between them: the web app uses discojs-web and is not part of it.

I get your point, but having many packages for small fonctionnalities is cumbersome in JS' world. I try to weight how much we can simplify our dev workflow and users workflow, and IMO merging some pkgs will improve it.

[JulienVig]

Does merging discojs-web into the webapp implies that we would need to re-build the webapp for changes to apply? (and lose the hot-reloading feature)

[tharvik]

after probing the code a bit more, it seems that discojs-core is already containing some node and web specific code (WebSocket implementation differs by platform, loading wrtc iff node). keeping this split (discojs-node & discojs-web) does makes more sense for me now.

Does merging discojs-web into the webapp implies that we would need to re-build the webapp for changes to apply? (and lose the hot-reloading feature)

no, as discojs-core will be a real deps of discojs-web (and not contained in it), hot-reloading seems to work with deps also, at least when trying out in #617: cd web-client && npm start seems to watch pretty much everything