-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feature(dspy): dspy-integration-with-langfuse #1186
base: main
Are you sure you want to change the base?
feature(dspy): dspy-integration-with-langfuse #1186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @xucailiang I have left some comments. I am learning DSPy too and just wondering whether the proposed integration could be done in a more elegant way
…e(dspy)-dspy-integration-with-langfuse
I have rewritten the code, the new calling method is more elegant, I realize that all LLM reasoning can be collected to langfuse, but it is difficult for me to test all the code. I have already tested my submission, maybe we can continue to integrate other LLM modules after this PR is merged |
…e(dspy)-dspy-integration-with-langfuse
…e(dspy)-dspy-integration-with-langfuse # Conflicts: # dsp/modules/azure_openai.py
…e(dspy)-dspy-integration-with-langfuse
@xucailiang hey! I will check it out this weekend |
Thanks for your time, I'm not sure if we'll wrap up this pull request over the weekend. If you have any suggestions or ideas to improve this, please feel free to share them with me at your earliest convenience. I'd really appreciate it! |
hi,I wonder if this discussion is still ongoing? |
+1, also interested in this! |
Hi! Unfortunately I have not had time to run it locally yet. I will do it. However, I am not a DSPy maintainer, just another user, so all my comments are just my comments :) I wonder whether it would make sense to ping one of the contributers so that they can take a look as well |
@arnavsinghvi11 hi,I was wondering if you could review this PR so I can continue integrating langfuse on other modules.Thank you. |
…e(dspy)-dspy-integration-with-langfuse
Hi @xucailiang , thanks for opening this PR! Exciting to see the Langfuse integration with DSPy. The PR looks on track so far, but I agree with your point that it's difficult to add this for all LM providers. I would suggest exploring ways to wrap the Langfuse logic not to each provider class but rather with the base lm.py class so any user using any LM provider can declare the langfuse tracker as an arg and then have it popped before the LM request is made. Let me know if that makes sense and if you have any additional questions. Feel free to mention me here again when the PR is ready to review as well! |
…e(dspy)-dspy-integration-with-langfuse
Indeed, I have modified some of the code for this, I am glad you pointed out this oversight. And what I want to discuss is, do you mean that you want to put the tracker in lm's self.kwargs? However, I put the tracker outside of self.kwargs because I didn't think it was a necessary parameter for LLM. So we don't have to pop it. Or is there something extra I don't know about? |
….__class__, BaseTracker))
As mentioned in issue #1179 , I wrote some code to integrate langfuse.
when configured correctly, It will support the display of data from OpenAI, AzureOpenAI and Ollama on the Langfuse dashboard