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 extra params from kwargs sent to vllm pydantic model #1012

Merged
merged 7 commits into from
May 15, 2024

Conversation

omkar-sh
Copy link
Contributor

Can you just checkout and suggest any changes?

@DamianB-BitFlipper
Copy link

Hi Omkar. I highly recommend using the dict pop method. It will make the code much simpler.

@omkar-sh
Copy link
Contributor Author

omkar-sh commented May 12, 2024

Hi Damian, Made the changes with the pop method.

@DamianB-BitFlipper
Copy link

Hey. I would recommend using a default value of None. It is more pythonic than the string "False".

@DamianB-BitFlipper
Copy link

DamianB-BitFlipper commented May 12, 2024

Also, a comment explaining why you do the pop in the first place would help. Possibly link the issue #974 in the comment.

@omkar-sh
Copy link
Contributor Author

I made these changes.

@DamianB-BitFlipper
Copy link

Sorry for the constant nits, but could you please add a space between the , and the None. Thanks.

@omkar-sh
Copy link
Contributor Author

Pushed the changes. Thanks

@DamianB-BitFlipper
Copy link

This looks good to me!

@arnavsinghvi11
Copy link
Collaborator

Thanks @omkar-sh ! made the small change of removing the comments mentioning the issue request.

@arnavsinghvi11 arnavsinghvi11 merged commit 9c1fff9 into stanfordnlp:main May 15, 2024
4 checks passed
@LorenzoMinto
Copy link

If system_prompt is not None it will also cause this issue . Added a PR to remove that from the kwargs as well.

@Wolfsauge
Copy link

@LorenzoMinto Thanks for mentioning that. I will follow up below the PR linked.

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.

5 participants