Address comments

nod-ai · Nov 27, 2024 · 8a6688c · 8a6688c
1 parent 2016aae
commit 8a6688c
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 34 deletions.
diff --git a/shortfin/CMakeLists.txt b/shortfin/CMakeLists.txt
@@ -171,8 +171,10 @@ if(SHORTFIN_BUNDLE_DEPS)
   # Enable spdlog shared library options so we can export it.
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DSPDLOG_SHARED_LIB -Dspdlog_EXPORTS")
   message(STATUS "Fetching bundled projects")
+  list(APPEND CMAKE_MESSAGE_INDENT "  ")
   FetchContent_MakeAvailable(fmt spdlog xtl xtensor)
   shortfin_pop_bundled_lib_options()
+  list(POP_BACK CMAKE_MESSAGE_INDENT)
 else()
   find_package(spdlog)
   find_package(xtensor)
@@ -251,12 +253,16 @@ shortfin_pop_bundled_lib_options()
 
 function(shortfin_check_tokenizers)
   # Make sure that rust/cargo is installed and usable.
+  # Consider switching this to a cached variable once the tokenizers_cpp project
+  # will accept an override vs running whatever is on the path. For now, just
+  # verify the path is sane as that is what will get used.
   find_program(SHORTFIN_CARGO_PATH NAMES cargo NO_CACHE)
   if(NOT SHORTFIN_CARGO_PATH)
     message(SEND_ERROR
       "Building with -DSHORTFIN_ENABLE_TOKENIZERS=ON requires cargo (Rust's build tool). "
       "Please follow Rust documentation to install. On Ubuntu, this can typically be accomplished with:\n"
-      "  sudo apt install rustup && rustup default stable"
+      "  sudo apt install rustup && rustup default stable\n"
+      "See https://www.rust-lang.org/tools/install"
     )
   endif()
 
@@ -317,9 +323,9 @@ if(SHORTFIN_BUILD_TESTS)
   endif()
   include(GoogleTest)
   enable_testing()
+  add_custom_target(shortfin_testdata_deps)
 endif()
 
-add_custom_target(shortfin_testdata_deps)
 add_subdirectory(src)
 
 if(SHORTFIN_BUILD_PYTHON_BINDINGS)

diff --git a/shortfin/build_tools/cmake/shortfin_testing.cmake b/shortfin/build_tools/cmake/shortfin_testing.cmake
@@ -20,6 +20,9 @@ function(shortfin_download_test_data)
     ""
     ${ARGN}
   )
+  if(NOT SHORTFIN_BUILD_TESTS)
+    return()
+  endif()
   if(NOT EXISTS "${_RULE_OUTPUT_FILE}")
     set(_stage_file "${_RULE_OUTPUT_FILE}.stage")
     message(STATUS "Downloading test data ${_RULE_URL} -> ${_RULE_OUTPUT_FILE}")
@@ -36,7 +39,7 @@ function(shortfin_download_test_data)
   set_property(
     TARGET shortfin_testdata_deps
     APPEND PROPERTY ADDITIONAL_CLEAN_FILES
-      "${CMAKE_CURRENT_BINARY_DIR}/tokenizer.json"
+      "${_RULE_OUTPUT_FILE}"
   )
 
   # And make us reconfigure if it isn't there.

diff --git a/shortfin/src/shortfin/components/tokenizers/CMakeLists.txt b/shortfin/src/shortfin/components/tokenizers/CMakeLists.txt
@@ -4,36 +4,38 @@
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-if(SHORTFIN_ENABLE_TOKENIZERS)
-    shortfin_cc_component(
-      NAME
-        shortfin_tokenizers
-      HDRS
-        tokenizers.h
-      SRCS
-        tokenizers.cc
-      DEFINES
-        SHORTFIN_HAVE_TOKENIZERS
-      COMPONENTS
-        shortfin_support
-      DEPS
-        tokenizers_cpp
-    )
-    set_property(GLOBAL APPEND
-      PROPERTY SHORTFIN_LIB_OPTIONAL_COMPONENTS
-      shortfin_tokenizers)
-    target_compile_definitions(shortfin_public_defs INTERFACE SHORTFIN_HAVE_TOKENIZERS)
+if(NOT SHORTFIN_ENABLE_TOKENIZERS)
+  return()
+endif()
 
-    # Download test data.
-    shortfin_download_test_data(
-      URL "https://huggingface.co/google-bert/bert-base-cased/resolve/main/tokenizer.json"
-      OUTPUT_FILE "${CMAKE_CURRENT_BINARY_DIR}/tokenizer.json"
-    )
+shortfin_cc_component(
+  NAME
+    shortfin_tokenizers
+  HDRS
+    tokenizers.h
+  SRCS
+    tokenizers.cc
+  DEFINES
+    SHORTFIN_HAVE_TOKENIZERS
+  COMPONENTS
+    shortfin_support
+  DEPS
+    tokenizers_cpp
+)
+set_property(GLOBAL APPEND
+  PROPERTY SHORTFIN_LIB_OPTIONAL_COMPONENTS
+  shortfin_tokenizers)
+target_compile_definitions(shortfin_public_defs INTERFACE SHORTFIN_HAVE_TOKENIZERS)
 
-    # Note that tests run from the binary dir of the project.
-    shortfin_gtest_test(
-      NAME shortfin_tokenizers_test
-      SRCS
-        tokenizers_test.cc
-    )
-endif()
+# Download test data.
+shortfin_download_test_data(
+  URL "https://huggingface.co/google-bert/bert-base-cased/resolve/cd5ef92a9fb2f889e972770a36d4ed042daf221e/tokenizer.json"
+  OUTPUT_FILE "${CMAKE_CURRENT_BINARY_DIR}/tokenizer.json"
+)
+
+# Note that tests run from the binary dir of the project.
+shortfin_gtest_test(
+  NAME shortfin_tokenizers_test
+  SRCS
+    tokenizers_test.cc
+)