-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Fix test_long_examples_validator #406
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
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.
Both work but change is better
"Both functions aim to add a space at the start of a string if it is not already present. However, there is a subtle difference in their implementation.
The first function checks if the first character of the string is a space using x[0] == " ". If it is not, it adds a space at the beginning using "" if x[0] == " " else " ". Note that this function assumes that the input string is not empty. If the input string is empty, this function will throw an IndexError.
The second function also checks if the first character of the string is a space using x[0] == " ". However, it also checks if the string is empty using x == "". If the string is empty, it simply returns the string as is. Otherwise, it adds a space at the beginning using "" if x == "" or x[0] == " " else " ".
Therefore, the second function is more robust as it handles the case where the input string is empty."
openai/validators.py
Outdated
@@ -423,7 +423,7 @@ def completions_space_start_validator(df): | |||
|
|||
def add_space_start(x): | |||
x["completion"] = x["completion"].apply( | |||
lambda x: ("" if x[0] == " " else " ") + x | |||
lambda x: ("" if x == "" or x[0] == " " else " ") + x |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Thank you for the suggestion. I have updated the PR. Also I changed the x
in the lambda to s
so that it does not clash with the input parameter x
.
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.
Looks great! I've verified that (as before) this makes the test pass when applied on top of b82a3f7.
Thanks for this! We've since rewritten the library entirely, so this change is no longer relevant. I'm sorry we didn't get to it sooner. |
Ah, actually this change is still relevant, the code has just moved. I'll be applying this change in a separate PR shortly. |
Currently
test_long_examples_validator
fails with the following error message:The root cause is that the
x
in the lambda can be an empty string.This PR fixes this issue by adding an extra check. It assumes that when the string is empty, no extra space is needed.
Such issue can be prevented by adding a CI to run the unit tests automatically. You may want to merge #203. I have also put up a CI PR tuliren#1 with all tests passing on my fork, and can re-create the PR against this repo if that's helpful.