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

Update greedy_causal_lm.cpp to read EOS Token #315

Merged
merged 20 commits into from
Apr 9, 2024

Conversation

anzr299
Copy link
Contributor

@anzr299 anzr299 commented Mar 20, 2024

Details: Made*changes to accommodate the dynamic EOS Token
Tickets: #277 132861

Made changes to accommodate the dynamic EOS Token
@pavel-esir pavel-esir self-requested a review March 22, 2024 11:39
@pavel-esir
Copy link
Contributor

could you please also update beam search sample? EOS token is specified in parameters for beam search
https://github.com/openvinotoolkit/openvino.genai/blob/master/text_generation/causal_lm/cpp/beam_search_causal_lm.cpp#L53

Copy link
Contributor

@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.

@anzr299 thx for the PR. Left some comments. Please also check why compilation fails

@anzr299 anzr299 requested a review from pavel-esir March 24, 2024 10:16
getting the rt_info from the tokenizer instead of the LLM.
Copy link
Contributor

@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.

Please pay attention to failure issues: add type declaration for SPECIAL_EOS_TOKEN and so on
image

Please look to the comments i left in the code. Please try to build locally and check if it works. If you have any questions i am ready to answer

text_generation/causal_lm/cpp/greedy_causal_lm.cpp Outdated Show resolved Hide resolved
text_generation/causal_lm/cpp/greedy_causal_lm.cpp Outdated Show resolved Hide resolved
@anzr299
Copy link
Contributor Author

anzr299 commented Mar 26, 2024

I am having some trouble building it locally and hence I continued pushing to check the build. I would require some assistance with building it.

@pavel-esir
Copy link
Contributor

I am having some trouble building it locally and hence I continued pushing to check the build. I would require some assistance with building it.

Yes, sure. Could you please share some details, what kind of errors do you face? When you follow this steps https://github.com/ilya-lavrenov/openvino.genai/tree/ct-beam-search/text_generation/causal_lm/cpp#linuxmacos when the problem appears? During configure or compile stage?
You can run them separately one after another

cmake -DCMAKE_BUILD_TYPE=Release -S ./ -B ./build/
cmake --build ./build/ -j

@pavel-esir pavel-esir self-assigned this Apr 4, 2024
@pavel-esir pavel-esir changed the title Update greedy_causal_lm.cpp Update greedy_causal_lm.cpp to read EOS Token Apr 4, 2024
@anzr299
Copy link
Contributor Author

anzr299 commented Apr 6, 2024

@pavel-esir I have made the changes and tested it locally, can you please review it now?

anzr299 added 3 commits April 6, 2024 21:37
Made changes according to the review. This is the latest commit.
Fixed the error Related to the tokenizer read_model
@anzr299
Copy link
Contributor Author

anzr299 commented Apr 7, 2024

@ilya-lavrenov @pavel-esir I have pushed the latest changes, I have fixed the error which appeared earlier regarding reading the model which was due to it being called before the path was set.
Although I have a question, does the SPECIAL_EOS_TOKEN have a fixed type of an integer?

Also, I have updated beam_causal_lm to include reading the EOS token from the model runtime info.

anzr299 added 3 commits April 7, 2024 21:46
Added comments and organised the code better
Changed Beam Search header to accommodate eos token in parameter and removed the comment regarding eos token not implemented
Added EOS token into the parameters
@anzr299
Copy link
Contributor Author

anzr299 commented Apr 8, 2024

@ilya-lavrenov @pavel-esir I have pushed the latest changes and fixed the problem, could you please review it now?

@ilya-lavrenov ilya-lavrenov self-assigned this Apr 8, 2024
@anzr299
Copy link
Contributor Author

anzr299 commented Apr 8, 2024

@ilya-lavrenov @apaniukov @pavel-esir The specific check that fails is because the model Qwen-7B-Chat's runtime information does not contain an EOS token. Hence the program responds with an error as expected saying that the EOS token is not found in the runtime information.

@ilya-lavrenov
Copy link
Contributor

Please, update OpenVINO tokenizers submodule https://github.com/openvinotoolkit/openvino.genai/tree/master/thirdparty to catch up latest commit openvinotoolkit/openvino_tokenizers#109 which fixes Qwen-7B-Chat

@anzr299
Copy link
Contributor Author

anzr299 commented Apr 8, 2024

I am still updating the tokenizers submodule.

@anzr299
Copy link
Contributor Author

anzr299 commented Apr 8, 2024

@ilya-lavrenov I have updated the tokenizer submodule to the latest commit.

@anzr299
Copy link
Contributor Author

anzr299 commented Apr 9, 2024

@ilya-lavrenov I have just merged changes from the main repo. I think the error regarding building openvino tokenizer only for speculative decoding and stable diffusions could be fixed with the latest commit.

@anzr299
Copy link
Contributor Author

anzr299 commented Apr 9, 2024

@ilya-lavrenov I am looking deeper into this issue. The tokenizer submodule wasn't updated and hence the error regarding the specific header file added in this commit: "openvinotoolkit/openvino_tokenizers@35d7dcb" appears which was added after the old submodule update

@anzr299
Copy link
Contributor Author

anzr299 commented Apr 9, 2024

@ilya-lavrenov Is there anything I can assist with in regards to the tokenizer issue?

@ilya-lavrenov
Copy link
Contributor

Hi @anzr299
I've ported fix for Qwen model to releases/2024/0 branch, because latest master is not compatible with 2024.0 which is used in stable diffusion tests.
So, please, update to use releases/2024/0

@anzr299
Copy link
Contributor Author

anzr299 commented Apr 9, 2024

Hi @anzr299 I've ported fix for Qwen model to releases/2024/0 branch, because latest master is not compatible with 2024.0 which is used in stable diffusion tests. So, please, update to use releases/2024/0

Done

@ilya-lavrenov ilya-lavrenov merged commit 72caf05 into openvinotoolkit:master Apr 9, 2024
14 checks passed
@ilya-lavrenov
Copy link
Contributor

thank you for the contribution!

@mlukasze mlukasze added this to the 2024.2 milestone Jun 3, 2024
@anzr299 anzr299 deleted the patch-1 branch January 8, 2025 09:05
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.

[Good First Issue]: causal_lm/cpp must read EOS token value from rt_info of openvino_tokenizer.xml
4 participants