-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add vllm chat completion endpoints and fix inspect_history #811
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
Conversation
dsp/modules/hf_client.py
Outdated
| self.kwargs |= kwargs | ||
| # kwargs needs to have model, port and url for the lm.copy() to work properly | ||
| self.kwargs.update({ | ||
| 'model': model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model is in fact included in the kwargs already, inherited through HFModel-> LM so we can remove this.
is there a use case for including the port and URL in the LM kwargs? seems like they are only used for setting the URL(s) and not in the model payload? feel free to remove if not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted on the model part. For url and port, basically is when calling lm.copy() (ex: while compiling a program with bootstrap), it is missing the url and port kwargs. As shown as below. Thus, requiring the port and url in the kwargs for the copy() to work properly.
in BootstrapFewShot._bootstrap_one_example(self, example, round_idx)
148 with dsp.settings.context(trace=[], **self.teacher_settings):
149 lm = dsp.settings.lm
--> 150 lm = lm.copy(temperature=0.7 + 0.001 * round_idx) if round_idx > 0 else lm
151 new_settings = dict(lm=lm) if round_idx > 0 else {}
153 with dsp.settings.context(**new_settings):
File [~/micromamba/envs/yh_hf/lib/python3.10/site-packages/dsp/modules/lm.py:108](http://192.168.77.2:8892/lab/tree/~/micromamba/envs/yh_hf/lib/python3.10/site-packages/dsp/modules/lm.py#line=107), in LM.copy(self, **kwargs)
105 kwargs = {**self.kwargs, **kwargs}
106 model = kwargs.pop("model")
--> 108 return self.__class__(model=model, **kwargs)
TypeError: HFClientVLLM.__init__() missing 1 required positional argument: 'port'
dsp/modules/hf_client.py
Outdated
|
|
||
|
|
||
| # get model_type | ||
| model_type = kwargs.get("model_type",None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's actually move this to the initialization layer so the user can specify the model_type when configuring the vLLM client, not during generation time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will include into the initialization layer.
dsp/modules/lm.py
Outdated
| for idx, (prompt, choices) in enumerate(reversed(printed)): | ||
| printing_value = "" | ||
|
|
||
| for idx, (prompt, choices) in enumerate(printed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason for this change? seems to have the same effect originally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently failing these tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included some of my testing with the inspect_history in the issue #784. Can you take a look, or I'm misunderstanding how it should works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for highlighting. Just realized that this is a bug where the printing_value is getting reset everytime in the loop. That should be all the error is and the skip condition is fine as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Let me know what else is needed to change. I've updated to the vllm initialization part, and fix the error, so the test should pass now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah can you revert the changes made to inspect_history here and just move the printing_value outside the for loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, updated and tested it works. Thank you. Now inspect_history is able to view n-number of histories with skip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch folks!
thanks for it!
|
Thanks @noobHappylife for the additions to vLLM. left a couple comments and should be good to merge following those comments and running |
1e30f67 to
ca4efef
Compare
|
Thanks @noobHappylife ! |
Related to issue #778 #784