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
add rwkv world tokenizer #14
Conversation
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.
General LGTM
@@ -71,6 +77,13 @@ endif () | |||
get_filename_component(TOKENIZERS_CPP_ROOT ${CMAKE_CURRENT_LIST_FILE} DIRECTORY) | |||
set(TOKENIZERS_CPP_CARGO_SOURCE_PATH ${TOKENIZERS_CPP_ROOT}/rust) | |||
|
|||
FetchContent_Declare( |
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 wonder the differences between fetch_content
and setting it as a 3rdparty in the repo
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.
There shouldn't be much difference, whether it's 3rd or FetchContent, both pull repositories online for building. Personally, I prefer this approach.
Thanks @BBuf |
@BBuf @Hzfengsy This commit introduces a regression on windows build and blocks our nightly package rebuild. Please see the detailed logs below:
Link to the nightly run: https://github.com/mlc-ai/package/actions/runs/6069714174/job/16464517741 I do not have any clear idea how to get it fixed as I don't have a windows machine, but usually we would prefer submodule to FetchContent for network stability concerns. |
@BBuf would be great if we can followup on this. In the meantime, we can pin tokenizer-cpp to an earlier version if needed for now. Indeed submodule would be better |
I will change it to the submodule approach as soon as possible. |
|
prepare for mlc-ai/mlc-llm#848