Skip to content
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

improve chat template processing #549

Merged
merged 24 commits into from
Jul 11, 2024

Conversation

pavel-esir
Copy link
Contributor

@pavel-esir pavel-esir commented Jun 24, 2024

TODO

  • - add chat template applying tests
  • - throw informative exception if Jinja2Cpp was not able to process template

ticket: CVS-143685

@pavel-esir pavel-esir force-pushed the improve_chat_template branch 4 times, most recently from 708ab4f to 73c44bb Compare June 24, 2024 20:52
@pavel-esir
Copy link
Contributor Author

please do not review, PR is not ready yet

@ilya-lavrenov ilya-lavrenov added this to the 2024.3 milestone Jun 25, 2024
@pavel-esir pavel-esir marked this pull request as ready for review June 26, 2024 10:06
@pavel-esir
Copy link
Contributor Author

PR is ready for review

@pavel-esir pavel-esir force-pushed the improve_chat_template branch 2 times, most recently from 209cb1f to 025dfd3 Compare June 26, 2024 10:48
src/cpp/src/group_beam_searcher.cpp Outdated Show resolved Hide resolved
atten_mask = new_atten_mask;
} else {
atten_mask = attention_mask;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attention mask and position ids are specific for stateful and static model runners, while they are not required for PA-based.
I would move it to model runners / pipelines to avoid complexity in generic LLM pipeline impl.

But maybe it should be done during migration to CB rails
CC @Wovchena

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no other way for now

src/cpp/src/tokenizer.cpp Outdated Show resolved Hide resolved
tests/python_tests/test_generate_api.py Outdated Show resolved Hide resolved
tests/python_tests/test_generate_api.py Outdated Show resolved Hide resolved
src/cpp/src/group_beam_searcher.cpp Outdated Show resolved Hide resolved
@ilya-lavrenov ilya-lavrenov self-assigned this Jun 26, 2024
tests/python_tests/test_generate_api.py Outdated Show resolved Hide resolved
tests/python_tests/test_generate_api.py Outdated Show resolved Hide resolved
// If header containt that typical expression we update template and
// extract system message manually from ChatHistory.
std::string header_with_slice = "{% if messages[0]['role'] == 'system' %}{% set loop_messages = messages[1:] %}{% set system_message = messages[0]['content'] %}";
std::string replacement_string = "{% if false %}{% set placeholder = false %}";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you extend your Jinja2Cpp issue to keep track of missing features

src/cpp/src/tokenizer.cpp Outdated Show resolved Hide resolved
src/cpp/src/tokenizer.cpp Outdated Show resolved Hide resolved
src/cpp/src/llm_pipeline.cpp Outdated Show resolved Hide resolved
src/cpp/src/llm_pipeline.cpp Outdated Show resolved Hide resolved
src/cpp/src/llm_pipeline.cpp Outdated Show resolved Hide resolved
src/cpp/src/group_beam_searcher.cpp Outdated Show resolved Hide resolved
src/cpp/src/group_beam_searcher.cpp Outdated Show resolved Hide resolved
src/cpp/src/tokenizer.cpp Outdated Show resolved Hide resolved
@pavel-esir pavel-esir force-pushed the improve_chat_template branch 2 times, most recently from 8641b8d to f54c1da Compare June 28, 2024 10:07
@pavel-esir pavel-esir requested a review from Wovchena July 5, 2024 08:14
Copy link
Contributor Author

@pavel-esir pavel-esir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed leftover commented lines

tests/python_tests/test_chat_generate_api.py Outdated Show resolved Hide resolved
tests/python_tests/test_chat_generate_api.py Outdated Show resolved Hide resolved
tests/python_tests/test_chat_generate_api.py Outdated Show resolved Hide resolved
src/cpp/include/openvino/genai/llm_pipeline.hpp Outdated Show resolved Hide resolved
src/cpp/src/group_beam_searcher.cpp Show resolved Hide resolved
src/python/py_generate_pipeline.cpp Outdated Show resolved Hide resolved
.github/workflows/genai_python_lib.yml Outdated Show resolved Hide resolved
src/cpp/src/greedy_decoding.cpp Outdated Show resolved Hide resolved
tests/python_tests/ov_genai_test_utils.py Outdated Show resolved Hide resolved
src/cpp/src/llm_pipeline_base.hpp Show resolved Hide resolved
tests/python_tests/test_chat_generate_api.py Outdated Show resolved Hide resolved
tests/python_tests/test_chat_generate_api.py Outdated Show resolved Hide resolved
tests/python_tests/test_chat_generate_api.py Show resolved Hide resolved
@Wovchena Wovchena linked an issue Jul 10, 2024 that may be closed by this pull request
src/cpp/src/llm_pipeline.cpp Outdated Show resolved Hide resolved
tests/python_tests/ov_genai_test_utils.py Outdated Show resolved Hide resolved
tests/python_tests/test_chat_generate_api.py Outdated Show resolved Hide resolved
tests/python_tests/ov_genai_test_utils.py Show resolved Hide resolved
src/cpp/include/openvino/genai/llm_pipeline.hpp Outdated Show resolved Hide resolved
src/cpp/include/openvino/genai/llm_pipeline.hpp Outdated Show resolved Hide resolved
src/cpp/src/group_beam_searcher.cpp Outdated Show resolved Hide resolved
pavel-esir and others added 3 commits July 10, 2024 17:28
@pavel-esir pavel-esir requested a review from Wovchena July 11, 2024 08:43
@pavel-esir pavel-esir force-pushed the improve_chat_template branch 2 times, most recently from 7f77f89 to e4e78ac Compare July 11, 2024 10:30
@Wovchena Wovchena added this pull request to the merge queue Jul 11, 2024
Merged via the queue into openvinotoolkit:master with commit 048d439 Jul 11, 2024
27 checks passed
@pavel-esir pavel-esir deleted the improve_chat_template branch July 11, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chat_sample.exe doesn't work well when using Llama-2-7b-chat-hf
3 participants