Skip to content

Conversation

@yuki-97
Copy link
Contributor

@yuki-97 yuki-97 commented Mar 26, 2025

What does this PR do ?

Now generate by vllm will contain 0 instead of correct pad_token_id.

This PR is to fix this by:
Pass correct pad_value_dict to BatchedDataDict.from_batches when combining generated results, then output_ids will be pad by pad_token_id instead of 0.

This PR also organize the tokenizer and corresponding config settings into get_tokenizer and configure_generation_config, and calling the two functions at beginning to get the correctly set up tokenizer and update config.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this 

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

@yuki-97 yuki-97 requested a review from SahilJain314 March 26, 2025 09:20
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 26, 2025
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Mar 26, 2025
@yuki-97 yuki-97 force-pushed the yukih/fix_padding branch from 79e8957 to 886db53 Compare March 31, 2025 05:05
@yuki-97 yuki-97 force-pushed the yukih/fix_padding branch 2 times, most recently from 658a33b to 3657084 Compare March 31, 2025 06:44
@yuki-97 yuki-97 added Run CICD and removed Run CICD labels Mar 31, 2025
@yuki-97 yuki-97 mentioned this pull request Mar 31, 2025
4 tasks
@yuki-97 yuki-97 force-pushed the yukih/fix_padding branch 3 times, most recently from cbaa4f0 to 0d3f20f Compare April 1, 2025 13:44
@yuki-97 yuki-97 added Run CICD and removed Run CICD labels Apr 1, 2025
@yuki-97 yuki-97 force-pushed the yukih/fix_padding branch 2 times, most recently from cf5097d to eb6d9c1 Compare April 1, 2025 14:39
@yuki-97 yuki-97 added Run CICD and removed Run CICD labels Apr 1, 2025
@yuki-97 yuki-97 force-pushed the yukih/fix_padding branch from eb6d9c1 to c67634c Compare April 2, 2025 03:05
@yuki-97 yuki-97 added Run CICD and removed Run CICD labels Apr 2, 2025
@yuki-97 yuki-97 force-pushed the yukih/fix_padding branch 2 times, most recently from a7b6c50 to c9d52cf Compare April 3, 2025 04:04
@yuki-97 yuki-97 added Run CICD and removed Run CICD labels Apr 3, 2025
parthchadha
parthchadha previously approved these changes Apr 3, 2025
@terrykong
Copy link
Contributor

Could you also update the guidance here?

https://github.com/NVIDIA/reinforcer/blob/c9d52cf7a0fd4065b6ea745b7c47204946fa1cbf/nemo_reinforcer/models/generation/interfaces.py#L50-L51

IIUC, the guidance now is something like:

Tokenizer does not have a pad_token_id. Please use the nemo_reinforcer.algorithms.utils.get_tokenizer(...) API which sets pad_token_id if absent.

yuki-97 added 3 commits April 3, 2025 19:11
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97 yuki-97 force-pushed the yukih/fix_padding branch from c9d52cf to b9642bf Compare April 4, 2025 02:23
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 4, 2025
@yuki-97 yuki-97 added Run CICD and removed Run CICD labels Apr 4, 2025
yuki-97 added 2 commits April 4, 2025 02:51
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97 yuki-97 force-pushed the yukih/fix_padding branch from b9642bf to 9e6a116 Compare April 4, 2025 02:52
@yuki-97 yuki-97 added Run CICD and removed Run CICD labels Apr 4, 2025
@yuki-97
Copy link
Contributor Author

yuki-97 commented Apr 4, 2025

Could you also update the guidance here?

https://github.com/NVIDIA/reinforcer/blob/c9d52cf7a0fd4065b6ea745b7c47204946fa1cbf/nemo_reinforcer/models/generation/interfaces.py#L50-L51

IIUC, the guidance now is something like:

Tokenizer does not have a pad_token_id. Please use the nemo_reinforcer.algorithms.utils.get_tokenizer(...) API which sets pad_token_id if absent.

sure! updated.

@terrykong terrykong merged commit 2b7280d into main Apr 7, 2025
11 checks passed
@terrykong terrykong deleted the yukih/fix_padding branch April 7, 2025 17:58
parthchadha pushed a commit that referenced this pull request Apr 11, 2025
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Co-authored-by: Terry Kong <terrycurtiskong@gmail.com>
Signed-off-by: Parth Chadha <pchadha@nvidia.com>
KiddoZhu pushed a commit that referenced this pull request May 6, 2025
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Co-authored-by: Terry Kong <terrycurtiskong@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants