-
Notifications
You must be signed in to change notification settings - Fork 200
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
Fix streaming hieroglyphs #1492
Fix streaming hieroglyphs #1492
Conversation
is it possible to add tests for streamer? |
Unfortunately, it was quite difficult to add calling python from c++ and be sure that environment is correct with necessary packages to convert with optimum-cli to I upgraded existing tests for streamer and ensure that streamed results are the same as from generate. |
@@ -361,25 +361,47 @@ def test_callback_batch_fail(callback): | |||
pipe.generate(['1', '2'], ov_genai.GenerationConfig(), callback) | |||
|
|||
|
|||
@pytest.mark.parametrize("callback", [print, user_defined_callback, lambda subword: print(subword)]) | |||
class StremerWithResults: |
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.
I think we need to move this to
openvino.genai/tests/python_tests/common.py
Lines 328 to 359 in b62145b
def run_llm_pipeline( | |
models_path : Path, | |
prompts: List[str], | |
generation_config : GenerationConfig, | |
use_cb : bool = False | |
) -> List[GenerationResult]: | |
properties = get_default_properties() | |
if use_cb: | |
properties['scheduler_config'] = SchedulerConfig() | |
ov_pipe = LLMPipeline(models_path, device='CPU', **properties) | |
generate_outputs : DecodedResults = ov_pipe.generate(inputs=prompts, generation_config=generation_config) | |
index = 0 | |
generation_results = [] | |
for _ in prompts: | |
generation_result = GenerationResult() | |
generation_result.m_generation_ids = generate_outputs.texts[index : index + generation_config.num_return_sequences] | |
# sequences_scores are available only for beam search case | |
if generation_config.is_beam_search(): | |
generation_result.m_scores = generate_outputs.scores[index : index + generation_config.num_return_sequences] | |
generation_results.append(generation_result) | |
index += generation_config.num_return_sequences | |
del ov_pipe | |
shutil.rmtree(models_path) | |
return generation_results |
because it covers significantly more cases (but limit it to cases when we have a single batch and num_return_sequences is 1)
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.
I agree. But it's present only in master. This PR is for release branch.
I will do that when cherry-pick to master.
tests/python_tests/test_vlm_api.py
Outdated
pipe.generate(prompt, generation_config=get_greedy(), streamer=streamer) | ||
result_from_streamer = [] | ||
res = pipe.generate(prompt, generation_config=get_greedy(), streamer=streamer) | ||
assert res.texts[0] == ''.join(result_from_streamer) |
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.
I think we don't need to duplicate it in VLM pipeline, because internally both VLM and LLM use the same TextStreamer
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.
This VLM model is a good example where this streamer was broken in the first place but since we were not comparing here we didn't catch it from the beginning. This assert comparisons costs us almost nothing, but can prevent from mistakes.
…10-llamafied_ov and all existing UTF8 problems
Co-authored-by: Ilya Lavrenov <[email protected]>
9cf1601
After the fix
CVS-159227