Skip to content

Conversation

@skucherlapati
Copy link
Contributor

Adds native support for claude wrapper

@insop
Copy link
Contributor

insop commented Mar 7, 2024

Thank you.

It would be great if this files is updated, setup.py for package requirement
Also it would good to add the documentation in this folder: docs/api/language_model_clients

@okhat
Copy link
Collaborator

okhat commented Mar 7, 2024

Thanks a lot! No don't update setup.py, this does not need to be a global requirement.

Instead the import should happen only if needed, e.g. inside __init___

@CShorten
Copy link
Collaborator

CShorten commented Mar 9, 2024

@insop could you please help me with the ruff check here? I tried with:

ruff check . --fix-only

@insop
Copy link
Contributor

insop commented Mar 9, 2024

@insop could you please help me with the ruff check here? I tried with:

ruff check . --fix-only

I am new to ruff as I am still use black.

But I gave it a try by merging this PR branch to current main a51aad7.

Here is what I found:

  • main branch a51aad7: no ruff issue
  • my test branch with this PR merged to main: has many ruff issues that are not part of the PR
    (I double checked my test merging branch)

It is appeared to be that there is some config file change or some type of cached information are playing here?

Sorry, I tried, not no answer to this.

@CShorten
Copy link
Collaborator

CShorten commented Mar 9, 2024

Ok, so good to merge this PR without passing the ruff test?

Lgtm otherwise, maybe before every release we run the ruff test on master until this is fully understood within the team going forward?

@isaacbmiller
Copy link
Collaborator

Looking into this now. Do not merge yet, the test failing is not CI fault

@isaacbmiller
Copy link
Collaborator

@CShorten How are you pushing to this pull request?

@isaacbmiller
Copy link
Collaborator

Okay I fixed the issues:

  1. You need to make the imports optional, as anthropic is not a required dependency. That is why the tests were failing
  2. Somehow, there was a deletion of a Ruff rule in pyproject.toml, which is where there were so many fixes.
    Sorry for the mess on your branch!

Good to merge whenever @okhat @CShorten

@CShorten
Copy link
Collaborator

CShorten commented Mar 9, 2024

Thank you so much @insop @isaacbmiller @skucherlapati @okhat! Really appreciate your contributions to this! Claude API in DSPy!! Super cool!

@CShorten CShorten merged commit 974e4ec into stanfordnlp:main Mar 9, 2024
@insop
Copy link
Contributor

insop commented Mar 9, 2024

Thank you so much @insop !! Really appreciate your contribution to this! Claude API in DSPy!! Super cool!

Thank you @CShorten, happy to see Claude API in DSPy as well.

@skucherlapati
Copy link
Contributor Author

Thanks for the help on getting this merged. Makes sense - I should have made it optional!

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