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

descriptor_database.cc:656] File already exists in database: onnx/onnx-ml.proto #15194

Closed
yurivict opened this issue Dec 26, 2023 · 1 comment
Labels

Comments

@yurivict
Copy link

yurivict commented Dec 26, 2023

PyTorch fails during run-time with this error:

E0000 00:00:1703612509.198109       1 descriptor_database.cc:656] File already exists in database: onnx/onnx-ml.proto
F0000 00:00:1703612509.198269       1 descriptor.cc:2160] Check failed: GeneratedDatabase()->Add(encoded_file_descriptor, size) 

This seems to be the same problem that was reported 7.5 years ago: #1941

There is a Debian patch mentioned in the above report:
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=721791;filename=protobuf-2.4.1-3.1.debdiff;msg=5

This patch was never applied upstream.
It seems to have helped someone in the August of this year.
And it also helped me now with this problem on FreeBSD 14.0.

My adaptation of this patch for protobuf-24.4 is below.

Could you please adapt this patch and fix this problem for good?

Thank you,
Yuri

--adaptation of the patch on FreeBSD--

--- src/google/protobuf/descriptor_database.cc.orig     2023-12-26 22:06:00 UTC
+++ src/google/protobuf/descriptor_database.cc
@@ -568,6 +568,16 @@ bool EncodedDescriptorDatabase::Add(const void* encode
                                     int size) {
   FileDescriptorProto file;
   if (file.ParseFromArray(encoded_file_descriptor, size)) {
+    std::pair<const void*, int> existing = index_->FindFile(file.name());
+    if (existing.first) {
+      if (existing.second == size && memcmp(existing.first, encoded_file_descriptor, size) == 0) {
+        // Contents match
+        return true;
+      }
+      else {
+        ABSL_LOG(ERROR) << "File descriptor " << file.name() << " is already registered, but descriptor contents are different";
+      }
+    }
     return index_->AddFile(file, std::make_pair(encoded_file_descriptor, size));
   } else {
     ABSL_LOG(ERROR) << "Invalid file descriptor data passed to "

--- src/google/protobuf/message.cc.orig 2023-12-26 22:09:28 UTC
+++ src/google/protobuf/message.cc
@@ -308,7 +308,7 @@ void GeneratedMessageFactory::RegisterFile(
 void GeneratedMessageFactory::RegisterFile(
     const google::protobuf::internal::DescriptorTable* table) {
   if (!files_.insert(table).second) {
-    ABSL_LOG(FATAL) << "File is already registered: " << table->filename;
+    //ABSL_LOG(FATAL) << "File is already registered: " << table->filename;
   }
 }
 
@@ -323,8 +323,8 @@ void GeneratedMessageFactory::RegisterType(const Descr
   // the mutex.
   mutex_.AssertHeld();
   if (!type_map_.try_emplace(descriptor, prototype).second) {
-    ABSL_DLOG(FATAL) << "Type is already registered: "
-                     << descriptor->full_name();
+    //ABSL_DLOG(FATAL) << "Type is already registered: "
+    //                 << descriptor->full_name();
   }
 }
@yurivict yurivict added the untriaged auto added to all issues by default when created. label Dec 26, 2023
@buchgr buchgr added the c++ label Jan 24, 2024
@esrauchg
Copy link

esrauchg commented Jan 24, 2024

Unfortunately the listed patch is dangerous: the multiple registrations of potentially different contents implies surprising skew which could cause locally correct code to break.

In theory it could be safe to ignore multiple registrations which were exactly identical but I think such a case is a 'this is suspicious and likely to break if you don't stop doing this' enough situation that its probably better to noisily fail. In the dynamic linking case like the 2016 issue linked it'll end up with multiple registrations where the descriptor may happen to match until you actually get skew, then it could break even though you didn't change anything in your code.

Basically any given file should just only be registered once: if there's PyTorch code that is registering the same file twice then it more likely is something to fix on the caller's side.

Hope that makes sense!

@googleberg googleberg removed the untriaged auto added to all issues by default when created. label Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants