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

Removed URL encoding of dataset_name #1035

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DeerShark
Copy link

@DeerShark DeerShark commented Dec 11, 2024

I removed some encoding logic, so there will be no problem of Chinese error reporting. The following issue is that it uses other packages to call the API. The original SDK should be correct...

Issues with querying Chinese datasets


Important

Removed URL encoding for dataset_name and dataset_run_name in langfuse/client.py to fix issues with non-ASCII characters.

  • Behavior:
    • Removed URL encoding for dataset_name in get_dataset(), get_dataset_runs(), and get_dataset_run().
    • Removed URL encoding for run_name in get_dataset_run().
    • Removed URL encoding for name in _fetch_prompt_and_update_cache().
  • Functions:
    • _url_encode() is no longer used in the above functions.

This description was created by Ellipsis for b1afc36. It will automatically update as commits are pushed.

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ DeerShark
❌ Yucolu


Yucolu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

Removed URL encoding of dataset names in the Langfuse Python client to fix issues with querying Chinese datasets and improve handling of non-ASCII characters.

  • Removed urllib.parse.quote encoding from dataset name parameters in langfuse/client.py
  • Modified dataset API endpoint calls to pass raw dataset names instead of URL-encoded versions
  • Change affects methods like get_dataset_items(), get_dataset_runs(), and get_dataset_prompts()
  • Potential impact on API compatibility should be monitored for any URL encoding expectations

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@marcklingen marcklingen requested a review from hassiebp December 11, 2024 12:07
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

(updates since last review)

This PR continues the work of removing URL encoding for dataset names in the Langfuse Python client, focusing on improving compatibility with non-ASCII characters.

  • Removed URL encoding for run_name parameter in get_dataset_run() method
  • Removed URL encoding for name parameter in _fetch_prompt_and_update_cache() method
  • Maintains backward compatibility while fixing Chinese character handling
  • Ensures consistent raw string handling across dataset-related API endpoints

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@hassiebp hassiebp left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Please see my comment on the issue. Let's continue the discussion there before going ahead with this PR. langfuse/langfuse#3714 (comment)

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.

3 participants