Skip to content
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

bug(dspy): removed system_prompt from kwargs sent to vllm pydantic model #1029

Closed
wants to merge 1 commit into from

Conversation

LorenzoMinto
Copy link

No description provided.

@Wolfsauge
Copy link

Hello @LorenzoMinto Thank you for your perseverance in trying to iron out this problem. I have run into the same issue and fixed it differently, see here:
#1025 (comment)

I then noticed the PR #1012 being in the works, despite having a different approach, thinking it would be sufficient.

Also, I am not thinking my "fix" is any better, but I decided to start commenting on this, because I feel there is something wrong that I have to create such a "fix" in the first place.

There are sampling parameters of vLLM stored in the same set of key words as some keywords used for only DSPy internal processing. This is asking for trouble down the road.

While I don't want to discourage anything about adding "system_prompt" to the elements which should be popped out here, I want to ask for discussion if that is not asking for further trouble down the road and if not a deeper fix is needed to separate DSPy internal parameters from sampling parameters, which will make vLLM choke. It is very strict with regard to the correctness of the requests it will process.

What do you think?

@tom-doerr
Copy link
Contributor

I merged this to my local dspy version and have the same issue, seems like another parameter vllm doesn't take

@tom-doerr tom-doerr mentioned this pull request May 19, 2024
@tom-doerr
Copy link
Contributor

@Wolfsauge I went ahead and created a pull request using your code, hope you don't mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants