feat: add serve config for teacher model in `ilab data generate` by alimaredia · Pull Request #1777 · instructlab/instructlab

alimaredia · 2024-07-18T09:19:49Z

Currently ilab data generate uses the configuration from the serve section of configuration file to serve the teacher model in the background.

In the case where a user is using a specific model only as the teacher model, the user would have to edit the serve section of the config with the teacher model's settings.

Since the teacher model and it's configuration is static, and the serve section is dynamic, the teacher model and it's configuration should live in the generate section of the configuration.

Giving the teacher model it's own configuration section also prevents the user from accidentally serving the teacher model, if they have not changed the serve section.

This PR is meant to be a starting point. Further work should evolve the configuration of the teacher model in ilab data generate to be similar to how the ilab model evaluate command configures the judge model for MT-Bench.

This PR also includes a commit that adds the --enable-serving-output flag to ilab data generate. This flag serves the same purpose as it does in ilab model evaluate.

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the
conventional commits.
Changelog updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

alimaredia · 2024-07-18T09:23:32Z

Adding the hold label, which will be removed after I'm done testing this PR by hand. Reviews are still welcome.

leseb

The change looks ok to me but I'm missing some context. Can you explain how is configured the teacher model? Thanks

russellb · 2024-07-18T12:10:31Z

I guess I just don’t feel this is necessary. Here’s how I think about it: something has to go in the serve config. What is the majority use case that is fair to expect should be in there? My thought: the teacher model config.

An alternative answer to that could be “the target model to be trained, for use with ilab chat to test model behavior.”

Is the latter what you were thinking?

I’ll also say that I don’t like “general inference use cases” as an answer here because I don’t think using ilab makes much sense for that vs running vllm directly (via systemd). I’m not sure what value ilab provides in that case.

alimaredia · 2024-07-18T12:11:28Z

@leseb The teacher model can be a separate model that specializes just generating the data from the taxonomy. One model you could use for example is Mixtral. This is similar to how we have a judge model just for MT-Bench in evaluation. The model is prometheus.

The teacher model could have it's own set of configuration, ex: vllm_args.

alimaredia · 2024-07-18T12:38:01Z

I guess I just don’t feel this is necessary. Here’s how I think about it: something has to go in the serve config. What is the majority use case that is fair to expect should be in there? My thought: the teacher model config.

An alternative answer to that could be “the target model to be trained, for use with ilab chat to test model behavior.”

Is the latter what you were thinking?

I’ll also say that I don’t like “general inference use cases” as an answer here because I don’t think using ilab makes much sense for that vs running vllm directly (via systemd). I’m not sure what value ilab provides in that case.

The alternative answer along with inference use cases is what I was thinking. Here's how I see the entire ilab workflow along with when models are served:

Download all the necessary models. These are the Base, Teacher, and Judge models.
Chat with the Base model on how much it knows the information you want it to know about. Right now chat reads from the serve config if the model is started up in the background.
Make changes to the taxonomy, adding new skills and knowledge.
Run data generation. Right now the model started up in the background also reads from the serve config.
Train the model. Here MT-Bench is used in multiphase training. I think @JamesKunstle is calling the exact same functions in model/evaluate.py for the base and judge model and not reading anything from the serve config.
Chat with the trained model to see what the model learned. As mentioned in step 2 right now chat reads from the serve config if the model is started up in the background but since it's derived from the base model, I think it's serving configuration should exactly be the same except for the model path.
Run evaluation. For MT-Bench the necessary base and judge model configuration happens within evaluate.py, not in the serve config.

As I mentioned above I think since the teacher model can be viewed as something static similar to the judge model, that's a good reason for it to have it's own config in the generate section.

@leseb @n1hility do you have anything to add about the inference use case?

russellb · 2024-07-18T13:25:50Z

OK - I buy that the base model is a better thing to assume stays in the base serve config. Thanks.

russellb · 2024-07-18T13:27:31Z

        return get_api_base(self.host_port)


+class _generate(BaseModel):


any reason you needed to move this? It makes it harder to see what actually changed.

If you feel it needs to move, I would generally recommend doing that type of refactoring in a dedicated commit that includes no functional changes. That way you can still see the real code changes in the commit series.

I moved it below the _serve classes because those need to be declared first in order for _generate to have a member of that type. Without this move I get a NameError whenever ilab is invoked.

I wish I didn't have to move the class, but the only thing that has changed in the _generate class is just the addition of the teacher_serve_config variable.

I think you can reference via string, as described here

class Foo(BaseModel): bar: 'Bar' class Bar(BaseModel): pass

(though I'm fine with the class being moved too)

russellb · 2024-07-18T13:28:33Z

+    taxonomy_base: StrictStr
+
+    # additional fields with defaults
+    teacher_serve_config: _serve


Can you please look into backwards compatibility? I haven't tested this, but I'm guessing that old configs will be broken by this?

My preference would be to use the default serve config if it is not specified in this section.

The addition of this in the _serve config would not make the data generate command able to work with older configs. Especially since the code change I made in generate.py now expects the teacher_serve_config.

Backwards compatibility within the older config wasn't something I considered when I made this PR simply because I thought there have been so many changes to the config that we're not expecting the older configs to be fowards compatible.

It's for this reason I think #1749 should include a check that causes ilab to error out if a config doesn't have the version field.

What I could do is add a check in generate.py to see if the config has the teacher_serve_config section in the generate section and if it doesn't fall back to using the serve section of the config. How does that sound?

That config versioning PR merged (#1749) -- are you planning to update this PR to bump the version in some way?

My understanding is that the config version if for the release and not meant to be changed between PRs before v0.18.0.

This is not meant to be a take on when the config version should be changed, just that it should not be changed before the v0.18.0 release.

booxter · 2024-07-18T21:31:42Z

+    taxonomy_path: StrictStr
+    taxonomy_base: StrictStr
+
+    # additional fields with defaults


The new section doesn't seem to have a default value set in the model. Should it be defined elsewhere, so that this comment doesn't apply to it? (But see below.)

booxter · 2024-07-18T21:57:09Z

+    taxonomy_base: StrictStr
+
+    # additional fields with defaults
+    teacher_serve_config: _serve


IMO the name for the new section is not ideal. I don't think it should have _config suffix (it's redundant - the context here is well known already). Perhaps even the _serve mid-prefix could also be dropped, to simplify the name even more. What exactly does it communicate to the user beyond what teacher name could already do?

booxter · 2024-07-18T21:59:52Z

            model=DEFAULTS.DEFAULT_MODEL,
            taxonomy_path=DEFAULTS.TAXONOMY_DIR,
            taxonomy_base=DEFAULTS.TAXONOMY_BASE,
+            teacher_serve_config=_serve(


The default section seems to be identical to what is used for serve section. I suggest to avoid the duplication of the code here and in lines 413+: put the default value for both sections in a variable, then refer from both locations.

(That said, I think these defaults belong to the model spec, see above.)

booxter · 2024-07-18T23:22:48Z

+    taxonomy_base: StrictStr
+
+    # additional fields with defaults
+    teacher_serve_config: _serve


I think you could specify a default value for the field, and make all the nested models provide reasonable defaults; then you wouldn't need to pass the teacher argument below in get_default_config. To see what I mean, refer to: https://gist.github.com/booxter/e1332bbe3a5344ee217409d133a13776

I believe this will tackle the backwards compatibility concern by Russell too.

booxter · 2024-07-19T16:58:27Z

Note: the last patch should be squashed before merging.

alimaredia · 2024-07-22T00:34:17Z

@booxter Ran the branch by hand, and serving worked as expected. I've removed the hold label.

mergify · 2024-07-22T13:52:30Z

This pull request has merge conflicts that must be resolved before it can be
merged. @alimaredia please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

Signed-off-by: Ali Maredia <amaredia@redhat.com>

Passing this flag ensures that the stdout and stderr of vLLM will be output when ilab data generate is run. This is useful for debugging purposes. Signed-off-by: Ali Maredia <amaredia@redhat.com>

If the teacher section is not included in the config.yaml and read by ilab, the value of ctx.obj.generate.teacher is set to the default value of teacher in _generate. Signed-off-by: Ali Maredia <amaredia@redhat.com>

My preference would be to adopt more disciplined handling of config file changes instead of waiting until some point in the future since I don't think it's a ton of extra work. However, I will just bow out of this review so I'm not blocking anything.

booxter

Thank you for quick respin.

booxter · 2024-07-22T15:42:14Z

+    chunk_word_count: PositiveInt = DEFAULTS.CHUNK_WORD_COUNT
+    # DEPRECATED: see sdg_scale_factor instead
+    # Left in place so that we can still detect and give a warning if its
+    # specified in an old configuraiton file.


configuration

- Add entry for --enable-serving-output flag for ilab data generate - Add entry for new teacher section in the generate section of the config Signed-off-by: Ali Maredia <amaredia@redhat.com>

alimaredia · 2024-07-22T20:41:50Z

@russellb could you approve this PR so it merges?

RobotSail · 2024-07-22T22:07:40Z

+        default=-1, deprecated="see 'sdg_scale_factor' instead", exclude=True
+    )
+    sdg_scale_factor: Optional[PositiveInt] = DEFAULTS.SDG_SCALE_FACTOR
+    output_dir: StrictStr = Field(default_factory=lambda: DEFAULTS.DATASETS_DIR)


You can remove these Fields as they were previously used to prevent the config values from being cached before we ran tests. Nowadays, each test will reset the home directories to point to the temporary path fixture.

Suggested change

output_dir: StrictStr = Field(default_factory=lambda: DEFAULTS.DATASETS_DIR)

output_dir: StrictStr = DEFAULTS.DATASETS_DIR

@RobotSail The entire _generate section was moved in this PR, and one addition was made, the addition of the teacher member.

The line you mentioned is the same as what's in main here (https://github.com/instructlab/instructlab/blob/main/src/instructlab/configuration.py#L266). To keep the scope of this PR focused I'd prefer to open up a follow up PR with that change, and possibly changes to any other variables in this section.

Makes sense, let's approve this then and then fix it later.

RobotSail

I have one comment but once that's addressed we can merge this.

RobotSail

LGTM

alimaredia requested review from leseb and russellb July 18, 2024 09:19

mergify Bot added the testing Relates to testing label Jul 18, 2024

alimaredia added this to the 0.18.0a3 milestone Jul 18, 2024

mergify Bot added the ci-failure PR has at least one CI failure label Jul 18, 2024

alimaredia added the hold In-progress PR. Tag should be removed before merge. label Jul 18, 2024

alimaredia force-pushed the data-generate-serve-config branch from 3f9d799 to ab070ae Compare July 18, 2024 09:32

mergify Bot removed the ci-failure PR has at least one CI failure label Jul 18, 2024

leseb reviewed Jul 18, 2024

View reviewed changes

russellb previously requested changes Jul 18, 2024

View reviewed changes

alinaryan modified the milestones: 0.18.0a3, 0.18.0a4 Jul 18, 2024

booxter suggested changes Jul 18, 2024

View reviewed changes

alimaredia force-pushed the data-generate-serve-config branch from ab070ae to fe85b64 Compare July 19, 2024 12:23

mergify Bot added the ci-failure PR has at least one CI failure label Jul 19, 2024

alimaredia force-pushed the data-generate-serve-config branch from fe85b64 to fc46dd4 Compare July 19, 2024 12:33

mergify Bot removed the ci-failure PR has at least one CI failure label Jul 19, 2024

alimaredia force-pushed the data-generate-serve-config branch from fc46dd4 to bbcdd17 Compare July 19, 2024 12:35

mergify Bot added the ci-failure PR has at least one CI failure label Jul 19, 2024

alimaredia force-pushed the data-generate-serve-config branch from bbcdd17 to bbd8824 Compare July 19, 2024 12:46

mergify Bot removed the ci-failure PR has at least one CI failure label Jul 19, 2024

booxter approved these changes Jul 19, 2024

View reviewed changes

alimaredia force-pushed the data-generate-serve-config branch from bbd8824 to 710190a Compare July 19, 2024 17:41

booxter approved these changes Jul 19, 2024

View reviewed changes

booxter requested a review from russellb July 19, 2024 23:21

alimaredia removed the hold In-progress PR. Tag should be removed before merge. label Jul 22, 2024

mergify Bot added the needs-rebase This Pull Request needs to be rebased label Jul 22, 2024

alinaryan modified the milestones: 0.18.0a4, 0.18.0a5 Jul 22, 2024

alimaredia added 3 commits July 22, 2024 15:32

feat: add serve config in generate for teacher model

d5cf0a2

Signed-off-by: Ali Maredia <amaredia@redhat.com>

feat: add --enable-serving-output flag to data generate cmd

4cf6dd5

Passing this flag ensures that the stdout and stderr of vLLM will be output when ilab data generate is run. This is useful for debugging purposes. Signed-off-by: Ali Maredia <amaredia@redhat.com>

feat: set defaults for teacher config

6d3f6fb

If the teacher section is not included in the config.yaml and read by ilab, the value of ctx.obj.generate.teacher is set to the default value of teacher in _generate. Signed-off-by: Ali Maredia <amaredia@redhat.com>

alimaredia force-pushed the data-generate-serve-config branch from 0c21abc to 6e05cfa Compare July 22, 2024 15:33

mergify Bot removed the needs-rebase This Pull Request needs to be rebased label Jul 22, 2024

russellb removed their request for review July 22, 2024 15:42

booxter approved these changes Jul 22, 2024

View reviewed changes

doc: edit CHANGLOG.md for SDG serving changes

e6744d2

- Add entry for --enable-serving-output flag for ilab data generate - Add entry for new teacher section in the generate section of the config Signed-off-by: Ali Maredia <amaredia@redhat.com>

alimaredia force-pushed the data-generate-serve-config branch from 6e05cfa to e6744d2 Compare July 22, 2024 15:44

booxter approved these changes Jul 22, 2024

View reviewed changes

alinaryan modified the milestones: 0.18.0a5, 0.18.0a6 Jul 22, 2024

RobotSail reviewed Jul 22, 2024

View reviewed changes

RobotSail approved these changes Jul 22, 2024

View reviewed changes

mergify Bot added the one-approval PR has one approval from a maintainer label Jul 22, 2024

khaledsulayman approved these changes Jul 22, 2024

View reviewed changes

mergify Bot merged commit 8a97762 into instructlab:main Jul 22, 2024

mergify Bot removed the one-approval PR has one approval from a maintainer label Jul 22, 2024

alimaredia mentioned this pull request Jul 29, 2024

set teacher model in generate to the value passed via --model #1901

Merged

5 tasks

alimaredia mentioned this pull request Aug 7, 2024

Check if a model server is already running when doing ilab data generate #1992

Closed

5 tasks

		return get_api_base(self.host_port)


		class _generate(BaseModel):

	output_dir: StrictStr = Field(default_factory=lambda: DEFAULTS.DATASETS_DIR)
	output_dir: StrictStr = DEFAULTS.DATASETS_DIR

Uh oh!

Conversation

alimaredia commented Jul 18, 2024

Uh oh!

alimaredia commented Jul 18, 2024

Uh oh!

leseb left a comment

Choose a reason for hiding this comment

Uh oh!

russellb commented Jul 18, 2024

Uh oh!

alimaredia commented Jul 18, 2024

Uh oh!

alimaredia commented Jul 18, 2024

Uh oh!

russellb commented Jul 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

booxter Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

booxter commented Jul 19, 2024

Uh oh!

alimaredia commented Jul 22, 2024

Uh oh!

mergify Bot commented Jul 22, 2024

Uh oh!

booxter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alimaredia commented Jul 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alimaredia Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobotSail left a comment

Choose a reason for hiding this comment

Uh oh!

RobotSail left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

booxter Jul 18, 2024 •

edited

Loading

alimaredia Jul 22, 2024 •

edited

Loading