fix: correct TOKENIZERS_PARALLELISM_ENV constant value#2596
fix: correct TOKENIZERS_PARALLELISM_ENV constant value#2596kuishou68 wants to merge 1 commit intovllm-project:mainfrom
Conversation
|
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA single-line bug fix in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request contains a change to the formatting of the TOKENIZERS_PARALLELISM_ENV constant in src/llmcompressor/entrypoints/oneshot.py. The review identifies that the removal of spaces around the assignment operator violates PEP 8 style guidelines and suggests reverting to the original spacing for better readability.
|
|
||
|
|
||
| TOKENIZERS_PARALLELISM_ENV = "TOKENIZERS_PARALLELISM" | ||
| TOKENIZERS_PARALLELISM_ENV="TOKENIZERS_PARALLELISM" |
There was a problem hiding this comment.
The assignment operator should be surrounded by spaces to adhere to PEP 8 style guidelines. Additionally, the string value in the proposed change is identical to the existing one in the provided diff, so the only effect is a style regression.
| TOKENIZERS_PARALLELISM_ENV="TOKENIZERS_PARALLELISM" | |
| TOKENIZERS_PARALLELISM_ENV = "TOKENIZERS_PARALLELISM" |
References
- PEP 8 recommends surrounding binary operators, including the assignment operator (=), with a single space on either side for better readability. (link)
|
The DCO sign-off has been added in the latest commit ( Could a maintainer add the |
f15126b to
4d97dbc
Compare
…roject#2595) The constant was set to 'tokenizers_parallelism' (lowercase) but the actual environment variable name used by the tokenizers library is 'TOKENIZERS_PARALLELISM' (uppercase). Fix the constant value and correct PEP8 spacing around the assignment. Signed-off-by: Cocoon-Break <54054995+kuishou68@users.noreply.github.com>
4d97dbc to
87a6d0e
Compare
brian-dellabetta
left a comment
There was a problem hiding this comment.
Hi @kuishou68 , thanks for raising this, but your PR changes seem unrelated to the summary. was there a bad merge resolution?
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 Require two reviewsWaiting for:
This rule is failing.PRs labelled "two-reviews" must have at least two approving reviews before merging.
|
|
This pull request has merge conflicts that must be resolved before it can be |
|
After rebasing, the cherry-pick of this fix results in an empty commit — the constant value is already correct in |
|
Hi @kuishou68 , thanks for the reply, yes i believe this is resolved by #2183 I will close this off, thanks |
Closes #2595
Problem
The constant
TOKENIZERS_PARALLELISM_ENVinsrc/llmcompressor/entrypoints/oneshot.pyhad a corrupted/truncated value:This caused
Oneshot.__init__to set the wrong environment variable (TOKENI...LISMinstead ofTOKENIZERS_PARALLELISM), meaning the HuggingFace tokenizer parallelism warning was never actually suppressed — defeating the original fix from #2183.Fix
Changed the constant to the correct string: