ilab command redesign - #990
Conversation
|
This pull request has merge conflicts that must be resolved before it can be |
hickeyma
left a comment
There was a problem hiding this comment.
This PR should be driven by design doc and not be merged until the design merges first.
790b995 to
6c052ec
Compare
883eea0 to
b96d96a
Compare
|
Add hold waiting on design doc |
|
Removed hold as design doc was merged. |
|
https://github.com/Mergifyio rebase |
☑️ Nothing to doDetails
|
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Charlie Doern <cdoern@redhat.com>
Previously, the config reading process and creation of the lab class could occur in lab.py. However, due to context passing norms in click, if we create the Lab object and read the ocnfig into the context when creating the lab group, we lose the context after passing it down one level of command. This means, we need to init our class object and context when at the sub-group level. In this commit I moved this functionality and the Lab class into config.py so that we do not need to duplicate code in each submodule Signed-off-by: Charlie Doern <cdoern@redhat.com>
russellb
left a comment
There was a problem hiding this comment.
There are now a bunch of follow-up issues open.
The ONLY thing I'd like to request changed at this point before merging is dropping the new prettytable dependency since it is no longer used.
| return | ||
| click.echo( | ||
| "Welcome to InstructLab CLI. This guide will help you to setup your environment." | ||
| "Welcome to InstructLab ilab. This guide will help you to setup your environment." |
There was a problem hiding this comment.
This gets changed back in a later commit, so I think it's OK ... bcf3c1d
Ideally the change would be brought back here, but it's not worth blocking on
|
|
||
|
|
||
|
|
||
| def init_config(ctx, config_file): |
| elif not os.path.isfile(config_file): | ||
| config_obj = None | ||
| ctx.fail( | ||
| f"`{config_file}` does not exists, please run `ilab init` " |
There was a problem hiding this comment.
... and should say "does not exist" instead of "exists"
but I don't think we need to block on this. It's pre-existing code and ilab init still works
| # the below library should NOT be imported into any python files | ||
| # it is for CLI usage ONLY | ||
| yamllint>=1.35.1,<1.36.0 | ||
| prettytable==3.10.0 |
There was a problem hiding this comment.
I think this change is worth making before merging
| def model(ctx, config_file): | ||
| """Command Group for Interacting with the Models in InstructLab. | ||
|
|
||
| If this is your first time running ilab, it's best to start with `ilab init` to create the environment. |
| import typing | ||
|
|
||
| # Third Party | ||
| from click_didyoumean import DYMGroup |
| def taxonomy(ctx): | ||
| """Command Group for Interacting with the Taxonomy of InstructLab. | ||
|
|
||
| If this is your first time running ilab, it's best to start with `ilab init` to create the environment. |
| Returns a torch.device object: | ||
| - type is one of 'cpu', 'cuda', 'hpu' | ||
| - index is None or device index (e.g. 0 for first GPU) | ||
| If this is your first time running ilab, it's best to start with `ilab init` to create the environment. |
| ilab.add_command(model_group.model) | ||
| ilab.add_command(taxonomy_group.taxonomy) | ||
| ilab.add_command(data_group.data) | ||
| ilab.add_command(config_group.config) |
There was a problem hiding this comment.
It looks like we're merging a bunch of commits that we know are in a broken state prior to this one. That's really not ideal, but I'm OK not blocking on it this time.
| if self.aliases: | ||
| formatter.write_paragraph() | ||
| formatter.write_text("Aliases:") | ||
| with formatter.indentation(): |
ilab now has a model command which has the following subcommands: ilab model chat ilab model convert ilab model serve ilab model test ilab model train ilab model download ilab model list is new, but simple and I think we should add it in with this redesign to let users get used to the idea of managing their models. Each child group needs to init their own Lab class due to context issues in click Signed-off-by: Charlie Doern <cdoern@redhat.com>
the data group only has one subcommand: ilab data generate Moved the file structure of generation so it makes more sense and lives within the data group. Signed-off-by: Charlie Doern <cdoern@redhat.com>
Add the config group and its one subcommand: ilab config init Removed the old init code. This code is still in the works as I am unsure of how to recitfy when/how to call the init_config comman from the config group Signed-off-by: Charlie Doern <cdoern@redhat.com>
the Taxonomy group has one command for now: ilab taxonomy diff ilab taxonomy download will be a future command create this command, the parent group, and remove the command in lab.py Signed-off-by: Charlie Doern <cdoern@redhat.com>
this commit brings all of the new commands back into lab.py, adds them to the ilab group, and cleans up the code by running the linting command. Now that the commands are added, all tests should still pass. I had to make a few adaptations to the unit tests specifically because most tests require --config=DEFAULT Signed-off-by: Charlie Doern <cdoern@redhat.com>
In order to alias properly, click requires a custom class that invokes a `get_command` function each time you call a click cmd. In my custom class, this function grabs a dictionary mapping cmd names to actual functions. These functions are then set to the actual cmd to be executed and returned. I also need to init the config here since we technically skip over the "sub-parent" in this scenario. For example, in `ilab model list` the `model` part is where the config is checked into the context. We skip over that here, so it needs to happen in this get_command func. Signed-off-by: Charlie Doern <cdoern@redhat.com>
Signed-off-by: Charlie Doern <cdoern@redhat.com>
|
thanks for catching the prettytable dependency @russellb , this should be all set now |
russellb
left a comment
There was a problem hiding this comment.
You have shown an impressive level of diligence as you worked through feedback. Thank you for sticking through this and pushing so hard to the end. Very nice work!
I'm good with merging this now!
|
I tried to apply I talked with @cdoern about the merge process and how we have Mergify configured to explicitly NOT merge when someone has previously requested changes and not come back and explicitly ACKed that they have been resolved. My preference would have been to ping both @markstur and @leseb to give them a chance to give that ACK. Only if they didn't respond would I consider dismissing the review. I know @cdoern had all the best of intentions here and genuinely felt that it was acceptable to dismiss the reviews if the feedback was addressed. In his defense, this is not documented anywhere! As I think about it, that's probably fine when the feedback in the review was simple enough. @leseb at least was doing more in depth review and has been very responsive on this. @cdoern has offered to revert the PR to give a chance for the final review responses. I told him I did not feel it was worth the churn in this case. @markstur @leseb would you both be OK taking another look at this PR (post-merge) to see if there was anything else you'd like to see addressed? If it's something not captured in a follow-up issue, please raise it. If you have any more significant concerns, we can consider reverting whatever portion is affected, but I'd rather hold off on that churn unless it's justified. Hopefully that makes sense to everyone! Let me know if I can help discuss anything further. |
yeah I was trying to ACK those when it merged. Now I'm looking at main. So far the concerns seem to be fixed or no longer relevant in what was merged |
|
@markstur @leseb I am really sorry for not letting you guys ACK the PR before merging it, it was an honest mistake as I genuinely thought stale reviews which I had either made follow up issues for or addressed in the PR, could be dismissed. I did not realize this was not a normal practice and will be careful doing it in the future. Please feel free to be honest with me and let me know if you'd like this reverted, I really apologize. |
@cdoern No worries. No revert needed for any of mine. Just double-checking to make sure they are resolved or no longer relevant in main. So far, so good. |
@cdoern No worries, I'm not offended by how things have unfolded. There's no need to revert; we have follow-ups in place, and any missed items will be addressed. |
add the following groups to ilab:
model
config
data
taxonomy
inside of these commands, there are groups commands that relate to the sub-command. For example
ilab model listlists all models known to the system.a full list of the commands in this PR is:
model
data
config
taxonomy
the goal of this is to make
ilabcommands more logical.ilab generatefor example is vague. also, adding structure allows for natural expansion of the project and makes the repo more digestible to new contributors.Grouping ilab commands under common resources which the cli deals with makes sense as the role of instructlab grows. This is the first step in making
ilaba model engine in which the cli will be a tool for full blown LLM management and deployment