fix: inputs of prompt path by trimming at end. #25
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Wallas Santos <wallashss@ibm.com>
ani300
reviewed
Apr 14, 2025
| parser.add_argument( | ||
| '--no_prompt_trim', | ||
| action="store_true", | ||
| help="Disable rtrip() from input prompts defined in --prompt_path. " |
ani300
reviewed
Apr 14, 2025
| if args.output_path != "": | ||
| output_path = Path(args.output_path) | ||
| output_path.mkdir(parents=True, exist_ok=True) | ||
| if output_path.is_dir(): | ||
| file_path = output_path / f"{result_idx}.txt" | ||
| with file_path.open("w", encoding="utf-8") as file: | ||
| file.write(output_str + "\n") | ||
| dprint(output_str) | ||
| dprint(f"prompt #{result_idx}: \n'{input_str}'") | ||
| dprint(f"generation #{result_idx}: \n'{output_str}'") |
There was a problem hiding this comment.
doesn't the output_str already contain the input_str?
ani300
reviewed
Apr 14, 2025
Comment on lines
+556
to
+563
| prompt_len = prompts_lens[result_idx] | ||
| if add_special_tokens: | ||
| prompt_len -= 1 | ||
| prompt = result[:prompt_len] | ||
| result = result[prompt_len:] | ||
| input_str = tokenizer.convert_tokens_to_string( | ||
| tokenizer.convert_ids_to_tokens(prompt) | ||
| ) |
There was a problem hiding this comment.
add comment to code why this needs to happen
ani300
reviewed
Apr 14, 2025
| result = generation.trim_prefix(result, tokenizer.bos_token_id) | ||
|
|
||
ani300
reviewed
Apr 14, 2025
| if has_padding: | ||
| result = generation.trim_prefix(result) | ||
|
|
||
ani300
reviewed
Apr 14, 2025
| @@ -460,11 +467,17 @@ def truncate_prompts_to_max_length(prompts, max_len, max_allowed_length): | |||
| len(prompt_file_paths) >= args.batch_size | |||
| ), f"Not enough prompt files at {prompt_path} for a batch size of {args.batch_size}" | |||
|
|
|||
| no_prompt_trim = args.no_prompt_trim | |||
There was a problem hiding this comment.
why define variable here if it's only used once?
ani300
requested changes
Apr 14, 2025
|
Hi @wallashss is this change still required and actively being worked on? Thank you |
|
Hey, these were just minor improvements that I thought the first time I used these scripts, I think now they were not so needed anymore and I don't have bandwidth to look those again. You can close this if you wish so. Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fix a problem of reproducibility on
inference.pywhen we set as input prompts from file with--prompt_path.The issue is due to POSIX files must be terminated with the new line character, so when the script reads the prompt of the file it does not remove it and it impacts the tokenization of the prompt. So, the solution is just trim right the prompt text (with rstrip()).
The default behavior is trim at right, but users can also disable this option with
no_prompt_trim, if they wish to.I also did a small change to the format of response, because I had to split the response that contains the prompt as well, and it was kinda of difficult to see them concatenated.