Skip to content

Conversation

@Anindyadeep
Copy link
Contributor

This PR adds a integration of premai python sdk to dspy modules.

Fixes issue: #1006

@Anindyadeep Anindyadeep marked this pull request as draft May 10, 2024 15:16
@Anindyadeep Anindyadeep marked this pull request as ready for review May 10, 2024 15:42
else:
self.client = premai.Prem(api_key=api_key)
self.provider = "premai"

Copy link
Collaborator

Choose a reason for hiding this comment

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

missing the line self.history: list[dict[str, Any]] = []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, but I wonder, self.history is also been initialized in the abstract base class, so why we also define it here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

defining the history here gives some flexibility with the typing (that is not done in the LM class). This behavior is maintained in most of the other LM classes (gpt3.py, Ollama, etc.).

Feel free to test the PremAI behavior with lm.inspect_history(n=1) on your end (where lm is defined as your dspy.PremAI model) as well.

@arnavsinghvi11
Copy link
Collaborator

Hi @Anindyadeep , thanks for the PR!

Added a few comments to resolve the failing build tests.

Could you also add documentation for the PremAI LM to provide context as done for the LMs in our documentation here?

Ready to merge after that!

@Anindyadeep
Copy link
Contributor Author

Hi @Anindyadeep , thanks for the PR!

Added a few comments to resolve the failing build tests.

Could you also add documentation for the PremAI LM to provide context as done for the LMs in our documentation here?

Ready to merge after that!

Hey thanks for doing the reviews, I will add the requested changes soon, also I would also like to discuss more on the pointers raised in replies.

@Anindyadeep
Copy link
Contributor Author

Hey @arnavsinghvi11, I added some of the changes and feedbacks you provided, but for some reason, I am not able to pass the lint tests (with pre-commit), even for files like lm.py where problems are getting raised for lines I have not changed. So what to do on those cases?

@Anindyadeep
Copy link
Contributor Author

Hi @arnavsinghvi11, it would great, if we can do one or two more iterations on this whenever you have some time.

Thanks

@arnavsinghvi11
Copy link
Collaborator

Hi @Anindyadeep , left a few comments.

Please refer to these lines in the failing tests on how to resolve and push the changes

https://github.com/stanfordnlp/dspy/actions/runs/9067260753/job/25022498634?pr=1007#step:5:10

https://github.com/stanfordnlp/dspy/actions/runs/9067260753/job/25022499093?pr=1007#step:8:28

@Anindyadeep
Copy link
Contributor Author

Anindyadeep commented May 16, 2024

Hi @arnavsinghvi11 , So, all tests passing now, and fixed some bugs, hopefully it should pass the ci workflow checks. For ruff, I got this:

vscode ➜ /workspaces/dspy (modules/premai) $ ruff check . --fix-only                                   
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'extend-unsafe-fixes' -> 'lint.extend-unsafe-fixes'
warning: Docstring at dspy/teleprompt/mipro_optimizer.py:111:9 contains implicit string concatenation; ignoring...
warning: Docstring at dspy/teleprompt/mipro_optimizer.py:122:9 contains implicit string concatenation; ignoring...

Which are nothing related to files on this integration.

@Anindyadeep Anindyadeep marked this pull request as draft May 16, 2024 15:59
@arnavsinghvi11
Copy link
Collaborator

Hi @arnavsinghvi11 , So, all tests passing now, and fixed some bugs, hopefully it should pass the ci workflow checks. For ruff, I got this:

vscode ➜ /workspaces/dspy (modules/premai) $ ruff check . --fix-only                                   
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'extend-unsafe-fixes' -> 'lint.extend-unsafe-fixes'
warning: Docstring at dspy/teleprompt/mipro_optimizer.py:111:9 contains implicit string concatenation; ignoring...
warning: Docstring at dspy/teleprompt/mipro_optimizer.py:122:9 contains implicit string concatenation; ignoring...

Which are nothing related to files on this integration.

Hi @Anindyadeep , seems like those are just warnings. the ruff check passes within the PR so ready to merge whenever you are ready!

@Anindyadeep
Copy link
Contributor Author

Hi @arnavsinghvi11 , So, all tests passing now, and fixed some bugs, hopefully it should pass the ci workflow checks. For ruff, I got this:

vscode ➜ /workspaces/dspy (modules/premai) $ ruff check . --fix-only                                   
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'extend-unsafe-fixes' -> 'lint.extend-unsafe-fixes'
warning: Docstring at dspy/teleprompt/mipro_optimizer.py:111:9 contains implicit string concatenation; ignoring...
warning: Docstring at dspy/teleprompt/mipro_optimizer.py:122:9 contains implicit string concatenation; ignoring...

Which are nothing related to files on this integration.

Hi @Anindyadeep , seems like those are just warnings. the ruff check passes within the PR so ready to merge whenever you are ready!

Thats, awesome, thanks I will ping you once, we are ready for merge.

@Anindyadeep Anindyadeep marked this pull request as ready for review May 18, 2024 05:31
@Anindyadeep
Copy link
Contributor Author

Hi @arnavsinghvi11, we are now ready get this merge.

@Anindyadeep
Copy link
Contributor Author

Hi @arnavsinghvi11, sorry have to bother you again, but it would be awesome, if you could approve this one, so that it checks the CI and we ready to merge

thanks :)

@arnavsinghvi11 arnavsinghvi11 merged commit 9dfb673 into stanfordnlp:main May 21, 2024
@arnavsinghvi11
Copy link
Collaborator

Thanks @Anindyadeep !

@Anindyadeep
Copy link
Contributor Author

Thanks @arnavsinghvi11 for the review :)

arnavsinghvi11 added a commit that referenced this pull request Jul 12, 2024
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.

2 participants