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.

Sign up for GitHub

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

Jump to bottom

DSPy Support For Snowflake LLM's #859

Merged
merged 27 commits into from
May 11, 2024
Merged

DSPy Support For Snowflake LLM's #859

merged 27 commits into from
May 11, 2024

Conversation

sfc-gh-alherrera
Copy link
Contributor

@sfc-gh-alherrera sfc-gh-alherrera commented Apr 17, 2024 •
edited
Loading

Adding support for using Snowflake's Cortex API with DSPy. This includes an implementation of a Snowflake LM as well as a Snowflake RM.

arnavsinghvi11
arnavsinghvi11 requested changes May 6, 2024
View reviewed changes
dspy/retrieve/snowflake_rm.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
dspy/retrieve/snowflake_rm.py Outdated Show resolved Hide resolved
@arnavsinghvi11
Copy link
Collaborator

arnavsinghvi11 commented May 6, 2024 •
edited
Loading

Thanks @sfc-gh-alherrera for the PR! left a few comments and seems like there are some merge conflicts to resolve.

Could you also add documentation for the Snowflake LM/RM to provide context as done for the LMs in our documentation here? and RMs.

@sfc-gh-alherrera
Copy link
Contributor Author

@arnavsinghvi11 thanks for taking a look. Just took a whole pass on your comments and addressed conflicts

arnavsinghvi11
arnavsinghvi11 requested changes May 11, 2024
View reviewed changes
docs/api/language_model_clients/Snowflake.md
### Usage

```python
lm = dspy.Snowflake(model="mixtral-8x7b",credentials=connection_parameters)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it would be useful to define some required connection_parameters from Snowflake based on the documentation so users know what to expect when configuring dspy.Snowflake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. added the same as in the RM docs

docs/api/retrieval_model_clients/SnowflakeRM.md
import os

connection_parameters = {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah you can add this to the LM documentation to address that comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@arnavsinghvi11
Copy link
Collaborator

left a few comments @sfc-gh-alherrera.

Can you also resolve the failing test from the actions on fixing the poetry.lock file? Additionally, can you run ruff check . --fix-only and push again to pass the failing test?

@sfc-gh-alherrera
Copy link
Contributor Author

sfc-gh-alherrera commented May 11, 2024 •
edited
Loading

thanks for the review @arnavsinghvi11. Just went through and addressed your latest round of comments.

@arnavsinghvi11 arnavsinghvi11 merged commit 822f932 into stanfordnlp:main May 11, 2024
4 checks passed
@arnavsinghvi11
Copy link
Collaborator

Thanks @sfc-gh-alherrera !

arnavsinghvi11 added a commit that referenced this pull request Jul 12, 2024
@arnavsinghvi11
Merge pull request #859 from sfc-gh-alherrera/dspy-snowflake
d5e96c1 
DSPy Support For Snowflake LLM's
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnavsinghvi11 arnavsinghvi11 Awaiting requested review from arnavsinghvi11

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sfc-gh-alherrera @arnavsinghvi11