diff --git a/tensorflow b/tensorflow index 16d39e94e37..9e76bf324f6 160000 --- a/tensorflow +++ b/tensorflow @@ -1 +1 @@ -Subproject commit 16d39e94e3724417fcaed87035434e098e892842 +Subproject commit 9e76bf324f6bac63137a02bb6e6ec9120703ea9b diff --git a/tensorflow_serving/apis/BUILD b/tensorflow_serving/apis/BUILD index 30c45a47b02..0e884e84dd7 100644 --- a/tensorflow_serving/apis/BUILD +++ b/tensorflow_serving/apis/BUILD @@ -21,7 +21,6 @@ filegroup( load("//tensorflow_serving:serving.bzl", "serving_proto_library") load("//tensorflow_serving:serving.bzl", "serving_proto_library_py") load("//tensorflow_serving:serving.bzl", "serving_go_grpc_library") -load("@org_tensorflow//tensorflow/core:platform/default/build_config.bzl", "tf_pyclif_proto_library") serving_proto_library( name = "get_model_metadata_proto", @@ -67,12 +66,6 @@ serving_proto_library_py( ], ) -tf_pyclif_proto_library( - name = "input_pyclif", - proto_lib = ":input_proto", - proto_srcfile = "input.proto", -) - serving_proto_library( name = "model_proto", srcs = ["model.proto"], @@ -91,12 +84,6 @@ serving_proto_library_py( deps = [], ) -tf_pyclif_proto_library( - name = "model_pyclif", - proto_lib = ":model_proto", - proto_srcfile = "model.proto", -) - serving_proto_library( name = "predict_proto", srcs = ["predict.proto"], @@ -178,12 +165,6 @@ serving_proto_library_py( ], ) -tf_pyclif_proto_library( - name = "classification_pyclif", - proto_lib = ":classification_proto", - proto_srcfile = "classification.proto", -) - serving_proto_library( name = "inference_proto", srcs = ["inference.proto"], @@ -210,12 +191,6 @@ serving_proto_library_py( ], ) -tf_pyclif_proto_library( - name = "inference_pyclif", - proto_lib = ":inference_proto", - proto_srcfile = "inference.proto", -) - serving_proto_library( name = "regression_proto", srcs = ["regression.proto"], @@ -239,12 +214,6 @@ serving_proto_library_py( ], ) -tf_pyclif_proto_library( - name = "regression_pyclif", - proto_lib = ":regression_proto", - proto_srcfile = "regression.proto", -) - cc_library( name = "classifier", hdrs = ["classifier.h"], diff --git a/tensorflow_serving/config/model_server_config.proto b/tensorflow_serving/config/model_server_config.proto index e69928fcdf1..864afdc4466 100644 --- a/tensorflow_serving/config/model_server_config.proto +++ b/tensorflow_serving/config/model_server_config.proto @@ -38,11 +38,7 @@ message ModelConfig { // (This cannot be changed once a model is in serving.) string model_platform = 4; - // DEPRECATED: This field is deprecated. For now it's still obeyed as long as - // 'model_version_policy' is not set. If 'model_version_policy' is set, then - // the value of this field is ignored. - FileSystemStoragePathSourceConfig.VersionPolicy version_policy = 5 - [deprecated = true]; + reserved 5; // Version policy for the model indicating how many versions of the model to // be served at the same time. diff --git a/tensorflow_serving/core/BUILD b/tensorflow_serving/core/BUILD index e1ef7755b8c..13bd550cbf9 100644 --- a/tensorflow_serving/core/BUILD +++ b/tensorflow_serving/core/BUILD @@ -87,6 +87,7 @@ cc_library( deps = [ ":loader", ":source_adapter", + "//tensorflow_serving/resources:resource_util", "//tensorflow_serving/resources:resource_values", "//tensorflow_serving/util:any_ptr", "//tensorflow_serving/util:optional", diff --git a/tensorflow_serving/core/loader.h b/tensorflow_serving/core/loader.h index 88d9fb631bc..b76c16cee34 100644 --- a/tensorflow_serving/core/loader.h +++ b/tensorflow_serving/core/loader.h @@ -71,7 +71,10 @@ class Loader { /// the estimate must specify the instance to which each resource is /// bound. /// 4. The estimate must be monotonically non-increasing, i.e. it cannot - /// increase over time. + /// increase over time. Reasons to have it potentially decrease over time + // include: (a) replace conservative estimate with actual measurement + // once loaded in memory; (b) load process consumes extra transient + // memory that is not used in steady-state after the load completes. /// /// @return an estimate of the resources the servable will consume once /// loaded. If the servable has already been loaded, returns an estimate of diff --git a/tensorflow_serving/core/simple_loader.h b/tensorflow_serving/core/simple_loader.h index 62452a36043..63c041681ff 100644 --- a/tensorflow_serving/core/simple_loader.h +++ b/tensorflow_serving/core/simple_loader.h @@ -26,6 +26,7 @@ limitations under the License. #include "tensorflow/core/platform/types.h" #include "tensorflow_serving/core/loader.h" #include "tensorflow_serving/core/source_adapter.h" +#include "tensorflow_serving/resources/resource_util.h" #include "tensorflow_serving/resources/resource_values.h" #include "tensorflow_serving/util/any_ptr.h" #include "tensorflow_serving/util/optional.h" @@ -62,6 +63,9 @@ namespace serving { // }; // std::unique_ptr loader(new SimpleLoader( // servable_creator, resource_estimator)); +// +// This class is not thread-safe. Synchronization is assumed to be done by the +// caller. template class SimpleLoader : public Loader { public: @@ -80,7 +84,19 @@ class SimpleLoader : public Loader { // and hence the serving system cannot enforce resource safety. static ResourceEstimator EstimateNoResources(); + // Constructor that takes a single resource estimator, to use for estimating + // the resources needed during load as well as post-load. SimpleLoader(Creator creator, ResourceEstimator resource_estimator); + + // Constructor that takes two resource estimators: one to use for estimating + // the resources needed during load, as well as a second one that gives a + // different estimate after loading has finished. See the documentation on + // Loader::EstimateResources() for (a) potential reasons the estimate might + // decrease, and (b) correctness constraints on how the estimate is allowed to + // change over time. + SimpleLoader(Creator creator, ResourceEstimator resource_estimator, + ResourceEstimator post_load_resource_estimator); + ~SimpleLoader() override = default; Status EstimateResources(ResourceAllocation* estimate) const override; @@ -94,11 +110,20 @@ class SimpleLoader : public Loader { private: Creator creator_; + // A function that estimates the resources needed to load the servable. ResourceEstimator resource_estimator_; - // The memoized estimated resource requirement of the session bundle servable. + // An optional function that estimates the resources needed for the servable + // after it has been loaded. (If omitted, 'resource_estimator_' should be used + // for all estimates, i.e. before, during and after load.) + optional post_load_resource_estimator_; + + // The memoized estimated resource requirement of the servable. mutable optional memoized_resource_estimate_; + std::unique_ptr resource_util_; + Resource ram_resource_; + std::unique_ptr servable_; TF_DISALLOW_COPY_AND_ASSIGN(SimpleLoader); @@ -180,7 +205,23 @@ SimpleLoader::EstimateNoResources() { template SimpleLoader::SimpleLoader(Creator creator, ResourceEstimator resource_estimator) - : creator_(creator), resource_estimator_(resource_estimator) {} + : creator_(creator), resource_estimator_(resource_estimator) { + ResourceUtil::Options resource_util_options; + resource_util_options.devices = {{device_types::kMain, 1}}; + resource_util_ = + std::unique_ptr(new ResourceUtil(resource_util_options)); + + ram_resource_ = resource_util_->CreateBoundResource( + device_types::kMain, resource_kinds::kRamBytes); +} + +template +SimpleLoader::SimpleLoader( + Creator creator, ResourceEstimator resource_estimator, + ResourceEstimator post_load_resource_estimator) + : SimpleLoader(creator, resource_estimator) { + post_load_resource_estimator_ = post_load_resource_estimator; +} template Status SimpleLoader::EstimateResources( @@ -198,8 +239,36 @@ Status SimpleLoader::EstimateResources( template Status SimpleLoader::Load() { - const Status status = creator_(&servable_); - return status; + TF_RETURN_IF_ERROR(creator_(&servable_)); + + if (post_load_resource_estimator_) { + // Save the during-load estimate (may be able to use the memoized value). + ResourceAllocation during_load_resource_estimate; + TF_RETURN_IF_ERROR(EstimateResources(&during_load_resource_estimate)); + + // Obtain the post-load estimate, and store it as the memoized value. + ResourceAllocation post_load_resource_estimate; + TF_RETURN_IF_ERROR( + (*post_load_resource_estimator_)(&post_load_resource_estimate)); + memoized_resource_estimate_ = post_load_resource_estimate; + + // Release any transient memory used only during load to the OS. + const uint64 during_load_ram_estimate = resource_util_->GetQuantity( + ram_resource_, during_load_resource_estimate); + const uint64 post_load_ram_estimate = + resource_util_->GetQuantity(ram_resource_, post_load_resource_estimate); + if (post_load_ram_estimate < during_load_ram_estimate) { + const uint64 transient_ram_estimate = + during_load_ram_estimate - post_load_ram_estimate; + LOG(INFO) << "Calling MallocExtension_ReleaseToSystem() after servable " + "load with " + << transient_ram_estimate; + ::tensorflow::port::MallocExtension_ReleaseToSystem( + transient_ram_estimate); + } + } + + return Status::OK(); } template @@ -219,14 +288,13 @@ void SimpleLoader::Unload() { // If we have a main-memory footprint estimate, release that amount of memory // to the OS. - for (const ResourceAllocation::Entry& entry : - resource_estimate.resource_quantities()) { - if (entry.resource().device() == device_types::kMain && - entry.resource().kind() == resource_kinds::kRamBytes) { - LOG(INFO) << "Calling MallocExtension_ReleaseToSystem() with " - << entry.quantity(); - ::tensorflow::port::MallocExtension_ReleaseToSystem(entry.quantity()); - } + const uint64 memory_estimate = + resource_util_->GetQuantity(ram_resource_, resource_estimate); + if (memory_estimate > 0) { + LOG(INFO) << "Calling MallocExtension_ReleaseToSystem() after servable " + "unload with " + << memory_estimate; + ::tensorflow::port::MallocExtension_ReleaseToSystem(memory_estimate); } } diff --git a/tensorflow_serving/core/simple_loader_test.cc b/tensorflow_serving/core/simple_loader_test.cc index 186b98cf922..da3d5b5632e 100644 --- a/tensorflow_serving/core/simple_loader_test.cc +++ b/tensorflow_serving/core/simple_loader_test.cc @@ -96,13 +96,69 @@ TEST(SimpleLoaderTest, ResourceEstimation) { *estimate = want; return Status::OK(); })); - for (int i = 0; i < 2; ++i) { + + { + ResourceAllocation got; + TF_ASSERT_OK(loader->EstimateResources(&got)); + EXPECT_THAT(got, EqualsProto(want)); + } + + // The estimate should remain the same after load. + TF_ASSERT_OK(loader->Load()); + { ResourceAllocation got; TF_ASSERT_OK(loader->EstimateResources(&got)); EXPECT_THAT(got, EqualsProto(want)); } } +TEST(SimpleLoaderTest, ResourceEstimationWithPostLoadRelease) { + const auto pre_load_resources = CreateProto( + "resource_quantities { " + " resource { " + " device: 'main' " + " kind: 'processing' " + " } " + " quantity: 42 " + "} "); + const auto post_load_resources = CreateProto( + "resource_quantities { " + " resource { " + " device: 'main' " + " kind: 'processing' " + " } " + " quantity: 17 " + "} "); + std::unique_ptr loader(new SimpleLoader( + [](std::unique_ptr* servable) { + servable->reset(new int); + return Status::OK(); + }, + [&pre_load_resources](ResourceAllocation* estimate) { + *estimate = pre_load_resources; + return Status::OK(); + }, + [&post_load_resources](ResourceAllocation* estimate) { + *estimate = post_load_resources; + return Status::OK(); + })); + + // Run it twice, to exercise memoization. + for (int i = 0; i < 2; ++i) { + ResourceAllocation got; + TF_ASSERT_OK(loader->EstimateResources(&got)); + EXPECT_THAT(got, EqualsProto(pre_load_resources)); + } + + // The estimate should switch to the post-load one after load. + TF_ASSERT_OK(loader->Load()); + { + ResourceAllocation got; + TF_ASSERT_OK(loader->EstimateResources(&got)); + EXPECT_THAT(got, EqualsProto(post_load_resources)); + } +} + // Verify that the error returned by the Creator is propagates back through // Load. TEST(SimpleLoaderTest, LoadError) { diff --git a/tensorflow_serving/g3doc/METADATA b/tensorflow_serving/g3doc/METADATA index 230bbb59486..819e6aaf439 100644 --- a/tensorflow_serving/g3doc/METADATA +++ b/tensorflow_serving/g3doc/METADATA @@ -1,6 +1,6 @@ name: "TensorFlow Serving" g3doc: { include: "/learning/serving/g3doc/METADATA" - sitemap_file: "/learning/serving/g3doc/users/sitemap.md" + sitemap_file: "/learning/serving/g3doc/sitemap.md" } diff --git a/tensorflow_serving/g3doc/setup.md b/tensorflow_serving/g3doc/setup.md index cffd48103ed..da9f1675424 100644 --- a/tensorflow_serving/g3doc/setup.md +++ b/tensorflow_serving/g3doc/setup.md @@ -182,7 +182,7 @@ in the documentation, you can add the flags `-c opt --copt=-msse4.1 subset of these flags). For example: ```shell -bazel build -c opt --config=mkl --copt=-msse4.1 --copt=-msse4.2 --copt=-mavx --copt=-mavx2 --copt=-mfma --copt=-O3 tensorflow_serving/... +bazel build -c opt --copt=-msse4.1 --copt=-msse4.2 --copt=-mavx --copt=-mavx2 --copt=-mfma --copt=-O3 tensorflow_serving/... ``` Note: These instruction sets are not available on all machines, especially with diff --git a/tensorflow_serving/g3doc/signature_defs.md b/tensorflow_serving/g3doc/signature_defs.md index 69d2cb1c42d..a68f192f7de 100644 --- a/tensorflow_serving/g3doc/signature_defs.md +++ b/tensorflow_serving/g3doc/signature_defs.md @@ -54,7 +54,7 @@ constants. Specifically: C++](https://github.com/tensorflow/tensorflow/blob/master/tensorflow/cc/saved_model/signature_constants.h). In addition, SavedModel provides a -[util](https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/saved_model/utils.py) +[util](https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/saved_model/signature_def_utils.py) to help build a signature-def. ## Sample structures diff --git a/tensorflow_serving/model_servers/BUILD b/tensorflow_serving/model_servers/BUILD index 5ee703c34ca..d4093db11ab 100644 --- a/tensorflow_serving/model_servers/BUILD +++ b/tensorflow_serving/model_servers/BUILD @@ -54,6 +54,7 @@ cc_library( deps = [ ":model_platform_types", "//tensorflow_serving/apis:model_proto", + "//tensorflow_serving/config:logging_config_proto", "//tensorflow_serving/config:model_server_config_proto", "//tensorflow_serving/config:platform_config_proto", "//tensorflow_serving/core:aspired_versions_manager", @@ -71,6 +72,7 @@ cc_library( "//tensorflow_serving/sources/storage_path:file_system_storage_path_source", "//tensorflow_serving/sources/storage_path:file_system_storage_path_source_proto", "//tensorflow_serving/util:event_bus", + "//tensorflow_serving/util:optional", "//tensorflow_serving/util:unique_ptr_with_deps", "@org_tensorflow//tensorflow/core:lib", "@protobuf//:cc_wkt_protos", @@ -82,6 +84,7 @@ cc_test( size = "medium", srcs = ["server_core_test.cc"], deps = [ + ":model_platform_types", ":server_core", "//tensorflow_serving/apis:model_proto", "//tensorflow_serving/apis:predict_proto", @@ -96,6 +99,8 @@ cc_test( "//tensorflow_serving/model_servers/test_util:storage_path_error_injecting_source_adapter", "//tensorflow_serving/model_servers/test_util:storage_path_error_injecting_source_adapter_proto", "//tensorflow_serving/test_util", + "@org_tensorflow//tensorflow/core:lib", + "@org_tensorflow//tensorflow/core:protos_all_cc", "@org_tensorflow//tensorflow/core:test", "@protobuf//:cc_wkt_protos", ], @@ -194,7 +199,7 @@ pkg_deb( homepage = "https://github.com/tensorflow/serving", maintainer = "TensorFlow Serving team", package = "tensorflow-model-server", - version = "undefined", # Set when releasing a new version of TensorFlow Serving (e.g. 1.0.0). + version = "1.3.0", # Set when releasing a new version of TensorFlow Serving (e.g. 1.0.0). ) # Build with '-c opt' @@ -205,5 +210,5 @@ pkg_deb( homepage = "https://github.com/tensorflow/serving", maintainer = "TensorFlow Serving team", package = "tensorflow-model-server-universal", - version = "undefined", # Set when releasing a new version of TensorFlow Serving (e.g. 1.0.0). + version = "1.3.0", # Set when releasing a new version of TensorFlow Serving (e.g. 1.0.0). ) diff --git a/tensorflow_serving/model_servers/main.cc b/tensorflow_serving/model_servers/main.cc index f097d24423a..6cad8011c24 100644 --- a/tensorflow_serving/model_servers/main.cc +++ b/tensorflow_serving/model_servers/main.cc @@ -87,8 +87,6 @@ using tensorflow::serving::AvailabilityPreservingPolicy; using tensorflow::serving::BatchingParameters; using tensorflow::serving::EventBus; using tensorflow::serving::FileSystemStoragePathSourceConfig; -using tensorflow::serving::FileSystemStoragePathSourceConfig_VersionPolicy; -using tensorflow::serving::FileSystemStoragePathSourceConfig_VersionPolicy_Name; using tensorflow::serving::GetModelMetadataImpl; using tensorflow::serving::ModelServerConfig; using tensorflow::serving::ServableState; diff --git a/tensorflow_serving/model_servers/server_core.cc b/tensorflow_serving/model_servers/server_core.cc index 762f69394de..db07a923d28 100644 --- a/tensorflow_serving/model_servers/server_core.cc +++ b/tensorflow_serving/model_servers/server_core.cc @@ -19,6 +19,7 @@ limitations under the License. #include "google/protobuf/any.pb.h" #include "google/protobuf/wrappers.pb.h" +#include "tensorflow/core/lib/io/path.h" #include "tensorflow/core/platform/logging.h" #include "tensorflow_serving/core/load_servables_fast.h" #include "tensorflow_serving/model_servers/model_platform_types.h" @@ -69,7 +70,9 @@ Status GetPlatform(const ModelConfig& model_config, string* platform) { // Returns an error if 'config_list' is invalid in some way, e.g. a model name // appearing multiple times. -Status ValidateModelConfigList(const ModelConfigList& config_list) { +Status ValidateModelConfigList(const ModelConfigList& config_list, + const ServerCore::Options& options) { + // Unique model-names. std::set model_names; for (const ModelConfig& config : config_list.config()) { if (model_names.find(config.name()) != model_names.end()) { @@ -79,6 +82,35 @@ Status ValidateModelConfigList(const ModelConfigList& config_list) { } model_names.insert(config.name()); } + + // Base-paths are either all relative, or all absolute. + using ::tensorflow::io::IsAbsolutePath; + using ::tensorflow::io::JoinPath; + if (options.model_config_list_root_dir) { + // All paths must be relative. + if (!IsAbsolutePath(*options.model_config_list_root_dir)) { + return errors::InvalidArgument(strings::StrCat( + "Expected non-empty absolute path; got model_config_list_root_dir=", + *options.model_config_list_root_dir)); + } + for (const ModelConfig& config : config_list.config()) { + if (IsAbsolutePath(config.base_path())) { + return errors::InvalidArgument(strings::StrCat( + "Expected model ", config.name(), + " to have a relative path; got base_path()=", config.base_path())); + } + } + } else { + // All paths must be absolute. + for (const ModelConfig& config : config_list.config()) { + if (!IsAbsolutePath(config.base_path())) { + return errors::InvalidArgument(strings::StrCat( + "Expected model ", config.name(), + " to have an absolute path; got base_path()=", config.base_path())); + } + } + } + return Status::OK(); } @@ -151,6 +183,32 @@ std::set NewModelNamesInSourceConfig( return new_models; } +// Updates the base_path fields in each ModelConfig, prepending an +// absolute model_config_list_root_dir. +// It is assumed that initially, all the base_path fields are relative. +Status UpdateModelConfigListRelativePaths( + const string& model_config_list_root_dir, ModelConfigList* config_list) { + using ::tensorflow::io::IsAbsolutePath; + using ::tensorflow::io::JoinPath; + std::vector updated_paths; + updated_paths.reserve(config_list->config_size()); + for (const ModelConfig& config : config_list->config()) { + updated_paths.emplace_back( + JoinPath(model_config_list_root_dir, config.base_path())); + if (!IsAbsolutePath(updated_paths.back())) { + return errors::InvalidArgument(strings::StrCat( + "Expected model ", config.name(), + " with updated base_path = JoinPath(", model_config_list_root_dir, + ", ", config.base_path(), ") to have an absolute path; got ", + updated_paths.back())); + } + } + for (int ii = 0; ii < updated_paths.size(); ++ii) { + config_list->mutable_config(ii)->set_base_path(updated_paths[ii]); + } + return Status::OK(); +} + } // namespace // ************************************************************************ @@ -363,7 +421,8 @@ Status ServerCore::ReloadConfig(const ModelServerConfig& new_config) { return Status::OK(); } if (new_config.config_case() == ModelServerConfig::kModelConfigList) { - TF_RETURN_IF_ERROR(ValidateModelConfigList(new_config.model_config_list())); + TF_RETURN_IF_ERROR( + ValidateModelConfigList(new_config.model_config_list(), options_)); } if (new_config.config_case() == ModelServerConfig::kModelConfigList && config_.config_case() == ModelServerConfig::kModelConfigList) { @@ -375,6 +434,11 @@ Status ServerCore::ReloadConfig(const ModelServerConfig& new_config) { LOG(INFO) << "Adding/updating models."; switch (config_.config_case()) { case ModelServerConfig::kModelConfigList: { + if (options_.model_config_list_root_dir) { + TF_RETURN_IF_ERROR(UpdateModelConfigListRelativePaths( + *options_.model_config_list_root_dir, + config_.mutable_model_config_list())); + } TF_RETURN_IF_ERROR(AddModelsViaModelConfigList()); break; } @@ -423,24 +487,7 @@ FileSystemStoragePathSourceConfig ServerCore::CreateStoragePathSourceConfig( source_config.add_servables(); servable->set_servable_name(model.name()); servable->set_base_path(model.base_path()); - // TODO(akhorlin): remove this logic once the corresponding deprecated - // field is removed (b/62834753). - if (!model.has_model_version_policy()) { - switch (model.version_policy()) { - case FileSystemStoragePathSourceConfig::LATEST_VERSION: - servable->mutable_servable_version_policy()->mutable_latest(); - break; - case FileSystemStoragePathSourceConfig::ALL_VERSIONS: - servable->mutable_servable_version_policy()->mutable_all(); - break; - default: - LOG(FATAL) << "Unknown version policy: " // Crash ok. - << model.version_policy(); - } - } else { - *servable->mutable_servable_version_policy() = - model.model_version_policy(); - } + *servable->mutable_servable_version_policy() = model.model_version_policy(); } return source_config; } diff --git a/tensorflow_serving/model_servers/server_core.h b/tensorflow_serving/model_servers/server_core.h index 341c512d9e6..53d941fc078 100644 --- a/tensorflow_serving/model_servers/server_core.h +++ b/tensorflow_serving/model_servers/server_core.h @@ -87,6 +87,9 @@ class ServerCore : public Manager { struct Options { // ModelServer configuration. ModelServerConfig model_server_config; + // Relative (non-absolute) base-paths in model_server_config will + // be prepended with model_config_list_root_dir. + optional model_config_list_root_dir; // The AspiredVersionPolicy to use for the manager. Must be non-null. std::unique_ptr aspired_version_policy; diff --git a/tensorflow_serving/model_servers/server_core_test.cc b/tensorflow_serving/model_servers/server_core_test.cc index 5583287d3dd..0941594652d 100644 --- a/tensorflow_serving/model_servers/server_core_test.cc +++ b/tensorflow_serving/model_servers/server_core_test.cc @@ -16,8 +16,10 @@ limitations under the License. #include "tensorflow_serving/model_servers/server_core.h" #include "google/protobuf/any.pb.h" +#include "tensorflow/core/lib/core/error_codes.pb.h" #include "tensorflow/core/lib/core/status.h" #include "tensorflow/core/lib/core/status_test_util.h" +#include "tensorflow/core/lib/io/path.h" #include "tensorflow_serving/apis/model.pb.h" #include "tensorflow_serving/apis/predict.pb.h" #include "tensorflow_serving/core/servable_handle.h" @@ -179,6 +181,98 @@ TEST_P(ServerCoreTest, ReloadConfigChangeModelBasePath) { available_servables.at(0).version != test_util::kTestModelVersion); } +class RelativePathsServerCoreTest : public ServerCoreTest { + protected: + // Creates a ModelServerConfig instance where the directory name has + // been stripped off the ModelConfig::base_path. Instead that prefix + // can be supplied via the Options::model_config_list_root_dir. + ModelServerConfig GetTestModelServerConfigWithRelativePath( + string* optional_config_list_root_dir = nullptr) { + using ::tensorflow::io::Basename; + using ::tensorflow::io::Dirname; + using ::tensorflow::io::IsAbsolutePath; + + ModelServerConfig result = GetTestModelServerConfigForFakePlatform(); + const string model_name = result.model_config_list().config(0).name(); + + ModelConfig& relative = + *result.mutable_model_config_list()->mutable_config(0); + relative.set_name(strings::StrCat(model_name, "_relative")); + CHECK(IsAbsolutePath(relative.base_path())) + << "relative.base_path()=" << relative.base_path() + << " must start as an absolute path for this unit-test to work."; + const string dirname = Dirname(relative.base_path()).ToString(); + const string basename = Basename(relative.base_path()).ToString(); + CHECK(!dirname.empty()); + CHECK(!basename.empty()); + relative.set_base_path(basename); + + if (optional_config_list_root_dir) { + *optional_config_list_root_dir = dirname; + } + + return result; + } +}; + +TEST_P(RelativePathsServerCoreTest, AbsolutePathSucceeds) { + std::unique_ptr server_core; + ModelServerConfig absolute = GetTestModelServerConfigForFakePlatform(); + TF_ASSERT_OK(CreateServerCore(absolute, &server_core)); +} + +TEST_P(RelativePathsServerCoreTest, RelativePathFails) { + std::unique_ptr server_core; + ModelServerConfig relative = GetTestModelServerConfigWithRelativePath(); + EXPECT_EQ(error::INVALID_ARGUMENT, + CreateServerCore(relative, &server_core).code()); +} + +TEST_P(RelativePathsServerCoreTest, AbsolutePathWithOptionsFails) { + std::unique_ptr server_core; + ModelServerConfig absolute = GetTestModelServerConfigForFakePlatform(); + ServerCore::Options options = GetDefaultOptions(); + { + string model_config_list_root_dir; + ModelServerConfig relative = + GetTestModelServerConfigWithRelativePath(&model_config_list_root_dir); + options.model_config_list_root_dir = std::move(model_config_list_root_dir); + } + EXPECT_EQ( + error::INVALID_ARGUMENT, + CreateServerCore(absolute, std::move(options), &server_core).code()); +} + +TEST_P(RelativePathsServerCoreTest, AbsolutePathWithEmptyPathFails) { + std::unique_ptr server_core; + ModelServerConfig absolute = GetTestModelServerConfigForFakePlatform(); + ServerCore::Options options = GetDefaultOptions(); + options.model_config_list_root_dir = ""; // This should fail IsAbsolutePath. + EXPECT_EQ( + error::INVALID_ARGUMENT, + CreateServerCore(absolute, std::move(options), &server_core).code()); +} + +TEST_P(RelativePathsServerCoreTest, RelativePathWithOptionsSucceeds) { + std::unique_ptr server_core; + ServerCore::Options options = GetDefaultOptions(); + string model_config_list_root_dir; + ModelServerConfig relative = + GetTestModelServerConfigWithRelativePath(&model_config_list_root_dir); + options.model_config_list_root_dir = std::move(model_config_list_root_dir); + TF_ASSERT_OK(CreateServerCore(relative, std::move(options), &server_core)); +} + +TEST_P(RelativePathsServerCoreTest, MixedAbsoluteRelativeFails) { + std::unique_ptr server_core; + ModelServerConfig mixed = GetTestModelServerConfigForFakePlatform(); + const ModelServerConfig relative = GetTestModelServerConfigWithRelativePath(); + *mixed.mutable_model_config_list()->add_config() = + relative.model_config_list().config(0); + EXPECT_EQ(error::INVALID_ARGUMENT, + CreateServerCore(mixed, &server_core).code()); +} + TEST_P(ServerCoreTest, ErroringModel) { ServerCore::Options options = GetDefaultOptions(); test_util::StoragePathErrorInjectingSourceAdapterConfig source_adapter_config; @@ -521,6 +615,10 @@ INSTANTIATE_TEST_CASE_P( TestType, ServerCoreTest, ::testing::Range(0, static_cast(ServerCoreTest::NUM_TEST_TYPES))); +INSTANTIATE_TEST_CASE_P( + TestType, RelativePathsServerCoreTest, + ::testing::Range(0, static_cast(ServerCoreTest::NUM_TEST_TYPES))); + } // namespace } // namespace serving } // namespace tensorflow diff --git a/tensorflow_serving/model_servers/test_util/BUILD b/tensorflow_serving/model_servers/test_util/BUILD index caee7eefaf0..94624d94ac7 100644 --- a/tensorflow_serving/model_servers/test_util/BUILD +++ b/tensorflow_serving/model_servers/test_util/BUILD @@ -69,6 +69,7 @@ cc_library( "//visibility:public", ], deps = [ + "//external:gtest", "//tensorflow_serving/core:availability_preserving_policy", "//tensorflow_serving/core:servable_id", "//tensorflow_serving/core/test_util:fake_loader_source_adapter", diff --git a/tensorflow_serving/model_servers/test_util/server_core_test_util.cc b/tensorflow_serving/model_servers/test_util/server_core_test_util.cc index 1805bf8677c..de523763537 100644 --- a/tensorflow_serving/model_servers/test_util/server_core_test_util.cc +++ b/tensorflow_serving/model_servers/test_util/server_core_test_util.cc @@ -70,12 +70,18 @@ ServerCore::Options GetDefaultOptions(const bool use_saved_model) { } // namespace Status CreateServerCore(const ModelServerConfig& config, + ServerCore::Options options, std::unique_ptr* server_core) { - ServerCore::Options options = GetDefaultOptions(true /*use_saved_model */); options.model_server_config = config; return ServerCore::Create(std::move(options), server_core); } +Status CreateServerCore(const ModelServerConfig& config, + std::unique_ptr* server_core) { + return CreateServerCore(config, GetDefaultOptions(true /*use_saved_model */), + server_core); +} + ModelServerConfig ServerCoreTest::GetTestModelServerConfigForFakePlatform() { ModelServerConfig config = GetTestModelServerConfigForTensorflowPlatform(); ModelConfig* model_config = @@ -117,8 +123,9 @@ ServerCore::Options ServerCoreTest::GetDefaultOptions() { } Status ServerCoreTest::CreateServerCore( - const ModelServerConfig& config, std::unique_ptr* server_core) { - return test_util::CreateServerCore(config, server_core); + const ModelServerConfig& config, ServerCore::Options options, + std::unique_ptr* server_core) { + return test_util::CreateServerCore(config, std::move(options), server_core); } } // namespace test_util diff --git a/tensorflow_serving/model_servers/test_util/server_core_test_util.h b/tensorflow_serving/model_servers/test_util/server_core_test_util.h index f8ffe135863..c936386ec9e 100644 --- a/tensorflow_serving/model_servers/test_util/server_core_test_util.h +++ b/tensorflow_serving/model_servers/test_util/server_core_test_util.h @@ -63,14 +63,27 @@ class ServerCoreTest : public ::testing::TestWithParam { ServerCore::Options GetDefaultOptions(); // Creates a ServerCore object configured with both a fake platform and the - // tensorflow platform, using GetDefaultOptions(). + // tensorflow platform, using the supplied options. Status CreateServerCore(const ModelServerConfig& config, + ServerCore::Options options, std::unique_ptr* server_core); + // Creates a ServerCore object configured with both a fake platform and the + // tensorflow platform, using GetDefaultOptions(). + Status CreateServerCore(const ModelServerConfig& config, + std::unique_ptr* server_core) { + return CreateServerCore(config, GetDefaultOptions(), server_core); + } + // Returns test type. This is the parameter of this test. TestType GetTestType() { return static_cast(GetParam()); } }; +// Creates a ServerCore object with the supplied options. +Status CreateServerCore(const ModelServerConfig& config, + ServerCore::Options options, + std::unique_ptr* server_core); + // Creates a ServerCore object with sane defaults. Status CreateServerCore(const ModelServerConfig& config, std::unique_ptr* server_core); diff --git a/tensorflow_serving/servables/tensorflow/BUILD b/tensorflow_serving/servables/tensorflow/BUILD index 93f3366308c..9648d188ec5 100644 --- a/tensorflow_serving/servables/tensorflow/BUILD +++ b/tensorflow_serving/servables/tensorflow/BUILD @@ -264,6 +264,8 @@ cc_library( "//tensorflow_serving/core:simple_loader", "//tensorflow_serving/core:source_adapter", "//tensorflow_serving/core:storage_path", + "//tensorflow_serving/resources:resource_util", + "//tensorflow_serving/resources:resource_values", "//tensorflow_serving/resources:resources_proto", "//tensorflow_serving/util:optional", "@org_tensorflow//tensorflow/cc/saved_model:loader", diff --git a/tensorflow_serving/servables/tensorflow/classifier_test.cc b/tensorflow_serving/servables/tensorflow/classifier_test.cc index 582ea12dd75..173543ab677 100644 --- a/tensorflow_serving/servables/tensorflow/classifier_test.cc +++ b/tensorflow_serving/servables/tensorflow/classifier_test.cc @@ -493,6 +493,31 @@ TEST_P(ClassifierTest, ScoresOnly) { " } ")); } +TEST_P(ClassifierTest, ZeroScoresArePresent) { + tensorflow::serving::Signatures signatures; + auto signature = signatures.mutable_default_signature() + ->mutable_classification_signature(); + signature->mutable_input()->set_tensor_name(kInputTensor); + // No classes Tensor. + signature->mutable_scores()->set_tensor_name(kScoreTensor); + TF_ASSERT_OK(tensorflow::serving::SetSignatures(signatures, meta_graph_def_)); + TF_ASSERT_OK(Create()); + auto* examples = + request_.mutable_input()->mutable_example_list()->mutable_examples(); + *examples->Add() = example({{"minus", -1}, {"zero", 0}, {"one", 1}}); + const std::vector expected_outputs = {-1, 0, 1}; + + TF_ASSERT_OK(classifier_->Classify(request_, &result_)); + // Parse the protos and compare the results with expected scores. + ASSERT_EQ(result_.classifications_size(), 1); + auto& classification = result_.classifications(0); + ASSERT_EQ(classification.classes_size(), 3); + + for (int i = 0; i < 3; ++i) { + EXPECT_NEAR(classification.classes(i).score(), expected_outputs[i], 1e-7); + } +} + TEST_P(ClassifierTest, ValidNamedSignature) { TF_ASSERT_OK(Create()); request_.mutable_model_spec()->set_signature_name(kOutputPlusOneSignature); diff --git a/tensorflow_serving/servables/tensorflow/saved_model_bundle_factory.h b/tensorflow_serving/servables/tensorflow/saved_model_bundle_factory.h index 9ab961a2e9f..df3c3419b20 100644 --- a/tensorflow_serving/servables/tensorflow/saved_model_bundle_factory.h +++ b/tensorflow_serving/servables/tensorflow/saved_model_bundle_factory.h @@ -73,6 +73,8 @@ class SavedModelBundleFactory { Status EstimateResourceRequirement(const string& path, ResourceAllocation* estimate) const; + const SessionBundleConfig& config() const { return config_; } + private: using Batcher = SharedBatchScheduler; diff --git a/tensorflow_serving/servables/tensorflow/saved_model_bundle_source_adapter.cc b/tensorflow_serving/servables/tensorflow/saved_model_bundle_source_adapter.cc index 8b4bbb2fff9..505219cdc58 100644 --- a/tensorflow_serving/servables/tensorflow/saved_model_bundle_source_adapter.cc +++ b/tensorflow_serving/servables/tensorflow/saved_model_bundle_source_adapter.cc @@ -21,6 +21,8 @@ limitations under the License. #include "tensorflow/core/lib/core/errors.h" #include "tensorflow/core/platform/types.h" #include "tensorflow_serving/core/simple_loader.h" +#include "tensorflow_serving/resources/resource_util.h" +#include "tensorflow_serving/resources/resource_values.h" #include "tensorflow_serving/resources/resources.pb.h" #include "tensorflow_serving/util/optional.h" @@ -52,10 +54,33 @@ Status SavedModelBundleSourceAdapter::Convert(const StoragePath& path, }; auto resource_estimator = [bundle_factory, path](ResourceAllocation* estimate) { + TF_RETURN_IF_ERROR( + bundle_factory->EstimateResourceRequirement(path, estimate)); + + // Add experimental_transient_ram_bytes_during_load. + // TODO(b/38376838): Remove once resource estimates are moved inside + // SavedModel. + ResourceUtil::Options resource_util_options; + resource_util_options.devices = {{device_types::kMain, 1}}; + std::unique_ptr resource_util = + std::unique_ptr(new ResourceUtil(resource_util_options)); + const Resource ram_resource = resource_util->CreateBoundResource( + device_types::kMain, resource_kinds::kRamBytes); + resource_util->SetQuantity( + ram_resource, + resource_util->GetQuantity(ram_resource, *estimate) + + bundle_factory->config() + .experimental_transient_ram_bytes_during_load(), + estimate); + + return Status::OK(); + }; + auto post_load_resource_estimator = [bundle_factory, + path](ResourceAllocation* estimate) { return bundle_factory->EstimateResourceRequirement(path, estimate); }; - loader->reset( - new SimpleLoader(servable_creator, resource_estimator)); + loader->reset(new SimpleLoader( + servable_creator, resource_estimator, post_load_resource_estimator)); return Status::OK(); } diff --git a/tensorflow_serving/servables/tensorflow/saved_model_bundle_source_adapter_test.cc b/tensorflow_serving/servables/tensorflow/saved_model_bundle_source_adapter_test.cc index 8dc6087d7f6..261b78e4eda 100644 --- a/tensorflow_serving/servables/tensorflow/saved_model_bundle_source_adapter_test.cc +++ b/tensorflow_serving/servables/tensorflow/saved_model_bundle_source_adapter_test.cc @@ -27,6 +27,8 @@ limitations under the License. #include "tensorflow/core/lib/core/status_test_util.h" #include "tensorflow_serving/core/loader.h" #include "tensorflow_serving/core/servable_data.h" +#include "tensorflow_serving/resources/resource_util.h" +#include "tensorflow_serving/resources/resource_values.h" #include "tensorflow_serving/resources/resources.pb.h" #include "tensorflow_serving/servables/tensorflow/bundle_factory_test_util.h" #include "tensorflow_serving/servables/tensorflow/session_bundle_config.pb.h" @@ -41,6 +43,16 @@ using test_util::EqualsProto; class SavedModelBundleSourceAdapterTest : public ::testing::Test { protected: + SavedModelBundleSourceAdapterTest() { + ResourceUtil::Options resource_util_options; + resource_util_options.devices = {{device_types::kMain, 1}}; + resource_util_ = + std::unique_ptr(new ResourceUtil(resource_util_options)); + + ram_resource_ = resource_util_->CreateBoundResource( + device_types::kMain, resource_kinds::kRamBytes); + } + void TestSavedModelBundleSourceAdapter( const SessionBundleSourceAdapterConfig& config, const string& export_dir) const { @@ -69,15 +81,33 @@ class SavedModelBundleSourceAdapterTest : public ::testing::Test { TF_ASSERT_OK(loader->Load()); + // We should get a new (lower) resource estimate post-load. + ResourceAllocation expected_post_load_resource_estimate = + first_resource_estimate; + resource_util_->SetQuantity( + ram_resource_, + resource_util_->GetQuantity(ram_resource_, first_resource_estimate) - + config.config().experimental_transient_ram_bytes_during_load(), + &expected_post_load_resource_estimate); + ResourceAllocation actual_post_load_resource_estimate; + TF_ASSERT_OK( + loader->EstimateResources(&actual_post_load_resource_estimate)); + EXPECT_THAT(actual_post_load_resource_estimate, + EqualsProto(expected_post_load_resource_estimate)); + const SavedModelBundle* bundle = loader->servable().get(); test_util::TestSingleRequest(bundle->session.get()); loader->Unload(); } + + std::unique_ptr resource_util_; + Resource ram_resource_; }; TEST_F(SavedModelBundleSourceAdapterTest, Basic) { - const SessionBundleSourceAdapterConfig config; + SessionBundleSourceAdapterConfig config; + config.mutable_config()->set_experimental_transient_ram_bytes_during_load(42); TestSavedModelBundleSourceAdapter(config, test_util::GetTestSavedModelPath()); } diff --git a/tensorflow_serving/servables/tensorflow/session_bundle_config.proto b/tensorflow_serving/servables/tensorflow/session_bundle_config.proto index 602819c88e0..69a3934f2b3 100644 --- a/tensorflow_serving/servables/tensorflow/session_bundle_config.proto +++ b/tensorflow_serving/servables/tensorflow/session_bundle_config.proto @@ -41,6 +41,16 @@ message SessionBundleConfig { // part of `session_config.session_inter_op_thread_pool`. google.protobuf.Int32Value session_run_load_threadpool_index = 4; + // EXPERIMENTAL. THIS FIELD MAY CHANGE OR GO AWAY. USE WITH CAUTION. + // + // Transient memory used while loading a model, which is released once the + // loading phase has completed. (This is on top of the memory used in steady- + // state while the model is in memory after it has finished loading.) + // + // TODO(b/38376838): This is a temporary hack, and it applies to all models. + // Remove it once resource estimates are moved inside SavedModel. + uint64 experimental_transient_ram_bytes_during_load = 5; + // EXPERIMENTAL. THIS FIELD MAY CHANGE OR GO AWAY. USE WITH CAUTION. // // Input tensors to append to every Session::Run() call. diff --git a/tensorflow_serving/sources/storage_path/file_system_storage_path_source.cc b/tensorflow_serving/sources/storage_path/file_system_storage_path_source.cc index 95dc15aef4d..582503a3ec9 100644 --- a/tensorflow_serving/sources/storage_path/file_system_storage_path_source.cc +++ b/tensorflow_serving/sources/storage_path/file_system_storage_path_source.cc @@ -132,6 +132,12 @@ IndexChildrenByVersion(const std::vector& children) { continue; } + if (children_by_version.count(version_number) > 0) { + LOG(WARNING) << "Duplicate version directories detected. Version " + << version_number << " will be loaded from " << children[i] + << ", " << children_by_version[version_number] + << " will be ignored."; + } children_by_version[version_number] = children[i]; } return children_by_version; diff --git a/tensorflow_serving/sources/storage_path/file_system_storage_path_source.proto b/tensorflow_serving/sources/storage_path/file_system_storage_path_source.proto index 8b11ce8237a..8fa7be5e55a 100644 --- a/tensorflow_serving/sources/storage_path/file_system_storage_path_source.proto +++ b/tensorflow_serving/sources/storage_path/file_system_storage_path_source.proto @@ -36,18 +36,6 @@ message FileSystemStoragePathSourceConfig { } } - // DEPRECATED: Please use VersionPolicy message definition instead. The - // enum will be removed once noone depends on it any longer. - // The policy to define how many versions of the servable should be - // served at the same time. - enum VersionPolicy { - // Only serve the latest version that exists in the base path. - // This is the default behavior. - LATEST_VERSION = 0; - // Serves all the versions that exist in the base path. - ALL_VERSIONS = 1; - } - // A servable name and base path to look for versions of the servable. message ServableToMonitor { // The servable name to supply in aspired-versions callback calls. Child diff --git a/tensorflow_serving/sources/storage_path/file_system_storage_path_source_test.cc b/tensorflow_serving/sources/storage_path/file_system_storage_path_source_test.cc index 2e52fb3436d..cc18864a67b 100644 --- a/tensorflow_serving/sources/storage_path/file_system_storage_path_source_test.cc +++ b/tensorflow_serving/sources/storage_path/file_system_storage_path_source_test.cc @@ -33,6 +33,7 @@ limitations under the License. #include "tensorflow_serving/sources/storage_path/file_system_storage_path_source.pb.h" #include "tensorflow_serving/test_util/test_util.h" +using ::testing::AnyOf; using ::testing::ElementsAre; using ::testing::Eq; using ::testing::IsEmpty; @@ -689,6 +690,43 @@ TEST(FileSystemStoragePathSourceTest, ServableVersionDirRenamed) { .PollFileSystemAndInvokeCallback()); } +TEST(FileSystemStoragePathSourceTest, DuplicateVersions) { + const string base_path = io::JoinPath(testing::TmpDir(), "DuplicateVersions"); + TF_ASSERT_OK(Env::Default()->CreateDir(base_path)); + TF_ASSERT_OK(Env::Default()->CreateDir( + io::JoinPath(base_path, "non_numerical_child"))); + for (const string& version : {"0001", "001", "00001"}) { + TF_ASSERT_OK(Env::Default()->CreateDir(io::JoinPath(base_path, version))); + } + + auto config = test_util::CreateProto( + strings::Printf("servable_name: 'test_servable_name' " + "base_path: '%s' " + // Disable the polling thread. + "file_system_poll_wait_seconds: -1 ", + base_path.c_str())); + std::unique_ptr source; + TF_ASSERT_OK(FileSystemStoragePathSource::Create(config, &source)); + std::unique_ptr target( + new StrictMock); + ConnectSourceToTarget(source.get(), target.get()); + + EXPECT_CALL( + *target, + SetAspiredVersions( + Eq("test_servable_name"), + AnyOf( + ElementsAre(ServableData( + {"test_servable_name", 1}, io::JoinPath(base_path, "001"))), + ElementsAre(ServableData( + {"test_servable_name", 1}, io::JoinPath(base_path, "0001"))), + ElementsAre(ServableData( + {"test_servable_name", 1}, + io::JoinPath(base_path, "00001")))))); + TF_ASSERT_OK(internal::FileSystemStoragePathSourceTestAccess(source.get()) + .PollFileSystemAndInvokeCallback()); +} + } // namespace } // namespace serving } // namespace tensorflow diff --git a/tensorflow_serving/tools/pip_package/setup.py b/tensorflow_serving/tools/pip_package/setup.py index fad0cff50ad..b94c7be8642 100644 --- a/tensorflow_serving/tools/pip_package/setup.py +++ b/tensorflow_serving/tools/pip_package/setup.py @@ -17,10 +17,10 @@ from setuptools import setup # Set when releasing a new version of TensorFlow Serving (e.g. 1.0.0). -_VERSION = 'undefined' +_VERSION = '1.3.0' REQUIRED_PACKAGES = [ - 'tensorflow>=1.2.0', + 'tensorflow>=1.3.0', 'grpcio>=1.0', ]