ilab model engine proposal - #48
Conversation
|
@cdoern can you fix the Markdown Lint/Spellcheck errors? |
|
@nathan-weinberg should be set |
nathan-weinberg
left a comment
There was a problem hiding this comment.
@cdoern Overall this proposal is very strong and I wholeheartedly agree with the approach - I have some minor requested changes but once they are addressed I will happily add my approval to the pile
| | |____convert | ||
| | |____download | ||
| | |____train (--convert) | ||
| | |____serve (-i) |
There was a problem hiding this comment.
| | |____serve (-i) | |
| | |____serve (-i) |
There was a problem hiding this comment.
Also, what is -i for @cdoern? Assume it's shorthand for interact or something of the like based on context clues elsewhere in the document (if I'm missing somewhere it's explicitly defined lmk)
There was a problem hiding this comment.
I assume it implies a new default of ilab serve launching model serving in the background, while -i or --interactive would keep it in the foreground?
There was a problem hiding this comment.
yep, I think I specify it implicitly below but will add a note.
|
|
||
| The primary focus here should be the `ilab model` group. The other ones should not be treated as blockers for launch, but would be nice to have. | ||
|
|
||
| Another key change that should be included is a switch to using `/var/lib/models` as a model store and `$HOME/.config/models/ilab.conf` for the new configuration path. Keeping config and models in the CWD does not map well to the importance of these two pieces. This will allow `ilab config` commands to act on a global level per-user. Models will be a system-wide directory as they are with most other applications of this type. |
There was a problem hiding this comment.
Agree with Charlie - I think we can start with a global config, but we can always enhance later for users to have options to create separate "projects" (there's some existing work for something like that here instructlab/instructlab#1022)
|
|
||
| ## Changes to Existing flow | ||
|
|
||
| The current `ilab` commands will still work. Users will be able to type commands like `ilab model train` or `ilab train` for the foreseeable future to ensure feature parity. Eventually, this alias should be removed, and only the sub-commands should probably exist. |
There was a problem hiding this comment.
We should target an explicit release to do so (outside of this general proposal, in our usual planning meetings/channels)
There was a problem hiding this comment.
we'll have to revisit having 2 ways of doing everything for sure. I guess it is comforting to suggest we a dual path for transition.
russellb
left a comment
There was a problem hiding this comment.
posting a few comments before a meeting starts, though I'm not actually finished with reviewing it
| | |____convert | ||
| | |____download | ||
| | |____train (--convert) | ||
| | |____serve (-i) |
There was a problem hiding this comment.
I assume it implies a new default of ilab serve launching model serving in the background, while -i or --interactive would keep it in the foreground?
| The main point of this structure is to introduce a group hierarchy and to improve the usability of the existing `ilab` structure. Some key things missing in `ilab` currently are: | ||
|
|
||
| 1. `ilab init` does too many things. | ||
| - We don't want "black box" commands. This creates bad UX and also ties too many processes together in which one might eventually need to be changed. |
There was a problem hiding this comment.
It's not clear that the proposal changes this at all. ilab init is just ilab config init. It seems like a distraction.
There was a problem hiding this comment.
Good point, this was an old reasoning anyway, will remove
|
|
||
| The primary focus here should be the `ilab model` group. The other ones should not be treated as blockers for launch, but would be nice to have. | ||
|
|
||
| Another key change that should be included is a switch to using `/var/lib/models` as a model store and `$HOME/.config/models/ilab.conf` for the new configuration path. Keeping config and models in the CWD does not map well to the importance of these two pieces. This will allow `ilab config` commands to act on a global level per-user. Models will be a system-wide directory as they are with most other applications of this type. |
There was a problem hiding this comment.
I would prefer to remove this from this proposal. Changing the CLI command structure is not dependent on making this configuration management change.
| vs. after: | ||
|
|
||
| ```console | ||
| ilab |
There was a problem hiding this comment.
I would say we should have those on an API resource level with OpenAPI and then have swagger to generate the CLI according. Otherwise, it would be very hard to make it api driven in the future,
There was a problem hiding this comment.
I'm also concerned we will want to restructure the CLI again once we have an API spec
There was a problem hiding this comment.
Maybe, but the API definition isn't going to happen quickly. This moves us much closer to what a resource-aligned command structure would look like that's more aligned with an API. I think it's a good move in the near term without APIs defined.
|
Overall I think this is a good proposal. It offers a good UX with lots of future expansion potential |
|
@russellb (GH wont let me quote reply)
I think this EP is meant to cover the cmd redesign sure but also the way ilab is thought about and generally used. That is why initially had the title as "model engine". Sure, we are redesigning the cmds, but we are also redesigning how we think about using ilab, its scope, etc. A lot of your comments seem to be removing some parts of that (the future structure, the ilab init stuff, etc). I think this is something I'd really like to leave, as well as the original title, and the future structure to be discussed in another EP as all of these things fit more with the mental model I was hoping to get across with this EP. Pushing up a new set of changes but hoping to roll back some of them if possible! lmk what you think. |
45af8e7 to
cd101e4
Compare
nathan-weinberg
left a comment
There was a problem hiding this comment.
I have a few outstanding comments but I agree with this document as a whole and personally am ready for us to move forward with implementation, so approving from my side
| | |____download | ||
| ``` | ||
|
|
||
| Note: -i will serve and start a chat process as opposed to the current flow which implicitly starts servers in certain commands |
There was a problem hiding this comment.
| Note: -i will serve and start a chat process as opposed to the current flow which implicitly starts servers in certain commands | |
| Note: `-i` will serve and start a chat process as opposed to the current flow which implicitly starts servers in certain commands |
There was a problem hiding this comment.
It's just a "Note:" which is fine but general thinking is that this -i feature for "serve" is a separate enhancement. Wouldn't want to pile on every to-do in this redesign.
There was a problem hiding this comment.
Agree that this is a separate feature
There was a problem hiding this comment.
sure, I can remove this
markstur
left a comment
There was a problem hiding this comment.
I went through and commented again on some of things that don't really jive with my way of thinking, but these are mostly just comments and not change requests. I know we are near the end of this. :)
But it is easier to comment critiques than bother noting the parts I agree with. To be clear... the argument might not be immediately compelling to me, but I do think we will benefit from the sub-command design eventually. So it's probably better to do it soon instead of using a bunch of flags (generate --data?) or commands like generate-data as work-arounds for subcommands. <-- that was me attempting to agree with you. :)
| | |____convert | ||
| | |____download | ||
| | |____train (--convert) | ||
| | |____serve (-i) |
| | |____download | ||
| ``` | ||
|
|
||
| Note: -i will serve and start a chat process as opposed to the current flow which implicitly starts servers in certain commands |
There was a problem hiding this comment.
It's just a "Note:" which is fine but general thinking is that this -i feature for "serve" is a separate enhancement. Wouldn't want to pile on every to-do in this redesign.
|
|
||
| The primary focus here should be the `ilab model` group. The other ones should not be treated as blockers for launch, but would be nice to have. | ||
|
|
||
| Another key change that should be included is a switch to using `/var/lib/models` as a model store and `$HOME/.config/models/ilab.conf` for the new configuration path. Keeping config and models in the CWD does not map well to the importance of these two pieces. This will allow `ilab config` commands to act on a global level per-user. Models will be a system-wide directory as they are with most other applications of this type. |
There was a problem hiding this comment.
worth noting that our "models" dir can (and should) just be links to the HF cache when applicable.
|
|
||
| The primary focus here should be the `ilab model` group. The other ones should not be treated as blockers for launch, but would be nice to have. | ||
|
|
||
| Another key change that should be included is a switch to using `/var/lib/models` as a model store and `$HOME/.config/models/ilab.conf` for the new configuration path. Keeping config and models in the CWD does not map well to the importance of these two pieces. This will allow `ilab config` commands to act on a global level per-user. Models will be a system-wide directory as they are with most other applications of this type. |
There was a problem hiding this comment.
re: global conf/models vs local dir -- I think ability to have a per-project conf and models is required and the ability to fallback to global is nice. So what that means is... we can add a global conf (default global models) but we have to have flags/config-settings to allow running independent projects in dirs. I suppose a config could have internal project sections, but I'm thinking more likely the local one is used first if it exists and the global is a fallback. Pretty common behavior.
|
|
||
| ### Necessity of a input -> configuration -> result mental model | ||
|
|
||
| The commands uncovered by adding this structure resemble the typical architecture for an "engine". `ilab` is not complete without the mechanisms to create, list, delete, and inspect the models. Models, as container images do in container engines, act as the configuration for the end result: the chat process. The interesting thing about this analogy, is that there needs to be a pre-cursor to configuration as well. There is the container image, the container, and the `Containerfile`. The `Containerfile` is the raw user input that leads to an image. |
There was a problem hiding this comment.
I still think references to "Containerfile" here are distracting and irrelevant to the discussion. Well-written, but less is more. Too late to re-suggest massive edits here though.
| 3. `ilab data generate` | ||
| 4. `ilab model train --convert` | ||
| 5. `ilab model serve -i` (or followed by `ilab model chat`) | ||
|
|
There was a problem hiding this comment.
So if I apply the enhancements like -i and ensure a server for generate (and chaining) it would be:
ilab init download generate train --convert serve -i. (maybe -i should be --chat)
So, removal of steps is separate from the redesign. I guess the argument here is that adding more words to the commands adds clarity. Not sure that's really good thing but...
I do, however, agree with the sub-module cli design so we're good.
There was a problem hiding this comment.
I think the -i bits have all been stripped from the enhancement
|
|
||
| ## Changes to Existing flow | ||
|
|
||
| The current `ilab` commands will still work. Users will be able to type commands like `ilab model train` or `ilab train` for the foreseeable future to ensure feature parity. Eventually, this alias should be removed, and only the sub-commands should probably exist. |
There was a problem hiding this comment.
we'll have to revisit having 2 ways of doing everything for sure. I guess it is comforting to suggest we a dual path for transition.
|
|
||
| ### click | ||
|
|
||
| click doesn't like the setup we currently have in `ilab`. So, adding sub-parent commands won't be as easy as creating a new `click.group`. We will need to make different libraries, each of which is a click group most likely. |
There was a problem hiding this comment.
If click is bad at this, is argparse better?
Also the "different libraries" comment is not clear to me and makes me concerned about how restructuring for click might be different than restructuring for future API or for general good structure.
There was a problem hiding this comment.
I think switching from click to argparse is a different convo, but one I want to have!!!1
In my draft PR I have gotten click to work with this structure, though the docs weren't super helpful lol
There was a problem hiding this comment.
@cdoern I second that using groups might not be straightforward and the current lab.py might require a bit of remodeling but I think the statement "adding sub-parent commands won't be as easy as creating a new click.group" is a bit misleading. Implementing something like ilab foo bar --baz is possible :). I'm not sure why we need to make different libraries. Thanks!
russellb
left a comment
There was a problem hiding this comment.
My biggest remaining concern is about dropping the discussion of the data and config store.
I would include the -i feature in a similar category of a new feature that seems separate from this. I would prefer removing that, too.
The other comments are just trivial things I saw while reading through again.
| docstring | ||
| podman | ||
| CLI | ||
| CLI's |
There was a problem hiding this comment.
This isn't something to block on, but if you have to update the PR anyway, it'd be good to run make spellcheck-sort.
| | |____download | ||
| ``` | ||
|
|
||
| Note: -i will serve and start a chat process as opposed to the current flow which implicitly starts servers in certain commands |
There was a problem hiding this comment.
Agree that this is a separate feature
a500ca9 to
2f6b888
Compare
This enhancement introduced a new design for `ilab`. Primarily adding sub-parent commands, new modes of interactions with `ilab`, and clarity on what the source + sink of data are in this system we are building Signed-off-by: Charlie Doern <cdoern@redhat.com>
| vs. after: | ||
|
|
||
| ```console | ||
| ilab |
There was a problem hiding this comment.
Maybe, but the API definition isn't going to happen quickly. This moves us much closer to what a resource-aligned command structure would look like that's more aligned with an API. I think it's a good move in the near term without APIs defined.
| 3. `ilab data generate` | ||
| 4. `ilab model train --convert` | ||
| 5. `ilab model serve -i` (or followed by `ilab model chat`) | ||
|
|
There was a problem hiding this comment.
I think the -i bits have all been stripped from the enhancement
This enhancement introduces a new design for
ilab. Primarily adding sub-parent commands, new modes of interactions withilab, and clarity on what the source + sink of data are in this system we are building.