From 158505edc3112f6dc68b3a17ffccb0a281a8e014 Mon Sep 17 00:00:00 2001 From: Mihai Maruseac Date: Tue, 30 Mar 2021 18:20:36 -0700 Subject: [PATCH 1/2] Switch TF filesystems to keep backwards compatibility. Recent changes to enable modular TF can result in breakages or require majority of users to take quick steps to fix their broken code. Instead, we should enable the modular filsystems only if requested by an environment variable and use the legacy behavior in all cases. In a future TF release, the cloud filesystems will come from different pip packages but we have to design the path towards that to ensure that no one in the TF ecosystem is broken by this. As we stand now, without this change most TF users will need to take action in a P0 basis and this is not desirable. CC @yongtang, @kvignesh1420, @vnvo2409. Note that I also changed the name of the envvar to match the logic and you'll have to do the same for SIG IO code. Let's sync on SIG IO meeting to discuss further PiperOrigin-RevId: 365940168 Change-Id: Id03b793613e521999af47fd6017b70ff659bd23b --- tensorflow/core/platform/env.h | 35 +++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/tensorflow/core/platform/env.h b/tensorflow/core/platform/env.h index 651ba423bb88a4..e573e94209f8f0 100644 --- a/tensorflow/core/platform/env.h +++ b/tensorflow/core/platform/env.h @@ -636,21 +636,22 @@ namespace register_file_system { template struct Register { - Register(Env* env, const std::string& scheme, bool legacy) { + Register(Env* env, const std::string& scheme, bool try_modular_filesystems) { // TODO(yongtang): Remove legacy file system registration for hdfs/s3/gcs // after TF 2.6+. - if (legacy) { - const char* enable_legacy_env = getenv("TF_ENABLE_LEGACY_FILESYSTEM"); - string enable_legacy = - enable_legacy_env ? absl::AsciiStrToLower(enable_legacy_env) : ""; - if (!(enable_legacy == "true" || enable_legacy == "1")) { + if (try_modular_filesystems) { + const char* env_value = getenv("TF_USE_MODULAR_FILESYSTEM"); + string load_plugin = env_value ? absl::AsciiStrToLower(env_value) : ""; + if (!(load_plugin == "true" || load_plugin == "1")) { + // We don't register the static filesystem and wait for SIG IO one + LOG(WARNING) << "Using modular file system for '" << scheme << "." + << " Please switch to tensorflow-io" + << " (https://github.com/tensorflow/io) for file system" + << " support of '" << scheme << "'."; return; } - LOG(WARNING) << "Legacy file system for '" << scheme << "' is deprecated" - << " and will be removed in tensorflow 2.6 or higher." - << " Please switch to tensorflow-io" - << " (https://github.com/tensorflow/io) for file system" - << " support of '" << scheme << "'."; + // If the envvar is missing or not "true"/"1", then fall back to legacy + // implementation to be backwards compatible. } // TODO(b/32704451): Don't just ignore the ::tensorflow::Status object! env->RegisterFileSystem(scheme, []() -> FileSystem* { return new Factory; }) @@ -666,15 +667,15 @@ struct Register { // Register a FileSystem implementation for a scheme. Files with names that have // "scheme://" prefixes are routed to use this implementation. -#define REGISTER_FILE_SYSTEM_ENV(env, scheme, factory, legacy) \ - REGISTER_FILE_SYSTEM_UNIQ_HELPER(__COUNTER__, env, scheme, factory, legacy) -#define REGISTER_FILE_SYSTEM_UNIQ_HELPER(ctr, env, scheme, factory, legacy) \ - REGISTER_FILE_SYSTEM_UNIQ(ctr, env, scheme, factory, legacy) -#define REGISTER_FILE_SYSTEM_UNIQ(ctr, env, scheme, factory, legacy) \ +#define REGISTER_FILE_SYSTEM_ENV(env, scheme, factory, modular) \ + REGISTER_FILE_SYSTEM_UNIQ_HELPER(__COUNTER__, env, scheme, factory, modular) +#define REGISTER_FILE_SYSTEM_UNIQ_HELPER(ctr, env, scheme, factory, modular) \ + REGISTER_FILE_SYSTEM_UNIQ(ctr, env, scheme, factory, modular) +#define REGISTER_FILE_SYSTEM_UNIQ(ctr, env, scheme, factory, modular) \ static ::tensorflow::register_file_system::Register \ register_ff##ctr TF_ATTRIBUTE_UNUSED = \ ::tensorflow::register_file_system::Register(env, scheme, \ - legacy) + modular) #define REGISTER_FILE_SYSTEM(scheme, factory) \ REGISTER_FILE_SYSTEM_ENV(::tensorflow::Env::Default(), scheme, factory, \ From b9e31e669e454b185a100802b5d6cb1a6f59d74b Mon Sep 17 00:00:00 2001 From: Mihai Maruseac Date: Wed, 31 Mar 2021 09:59:39 -0700 Subject: [PATCH 2/2] Fix typo/logic bug in modular plugins. We want to enable plugins only if the environment variable exists! CC @yongtang, @kvignesh1420, @vnvo2409. PiperOrigin-RevId: 366056903 Change-Id: Ia6f0918c97cbd6e064e19a02ce3a7d46f48b2799 --- tensorflow/core/platform/env.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorflow/core/platform/env.h b/tensorflow/core/platform/env.h index e573e94209f8f0..39a5eb92ef46c1 100644 --- a/tensorflow/core/platform/env.h +++ b/tensorflow/core/platform/env.h @@ -642,7 +642,7 @@ struct Register { if (try_modular_filesystems) { const char* env_value = getenv("TF_USE_MODULAR_FILESYSTEM"); string load_plugin = env_value ? absl::AsciiStrToLower(env_value) : ""; - if (!(load_plugin == "true" || load_plugin == "1")) { + if (load_plugin == "true" || load_plugin == "1") { // We don't register the static filesystem and wait for SIG IO one LOG(WARNING) << "Using modular file system for '" << scheme << "." << " Please switch to tensorflow-io"