Grounding dino requirements #6263

lucienfostier · 2025-02-08T19:57:21Z

GafferML::Inference : Add support for registering custom op library into ONNX.

Checklist

I have read the contribution guidelines.
I have updated the documentation, if applicable.
I have tested my change(s) in the test suite, and added new test cases where necessary.
My code follows the Gaffer project's prevailing coding style and conventions.

lucienfostier · 2025-02-08T19:58:04Z

@johnhaddon Could you have a look at this PR, it would be useful to deploy grounding dino through GafferML at IE.

This can be useful for models like Grounding Dino that requires the onnx runtime extension for the BERT tokenizer.

johnhaddon

Thanks Lucien! Would be great to get your dino stuff deployed at IE, and these seem like worthwhile features in general. I've commented inline as usual...
Cheers...
John

johnhaddon · 2025-02-11T10:20:02Z

src/GafferML/Tensor.cpp

@@ -213,6 +216,16 @@ Tensor::Tensor( const IECore::ConstDataPtr &data, std::vector<int64_t> shape )
 				std::copy( typedData->readable().begin(), typedData->readable().end(), value.GetTensorMutableData<bool>() );
 				m_state = new State{ std::move( value ), nullptr };
 			}
+			else if constexpr( std::is_same_v<DataType, StringVectorData> )
+			{
+				// Special case for the vector of std::string fiasco.


The "vector of bool fiasco" is a specific problem with vector<bool> that doesn't apply to strings. It looks like we do need a special case for strings, but not because of any underlying problems with vector<string>.

addressed here c7db62d

johnhaddon · 2025-02-11T10:23:16Z

src/GafferML/Tensor.cpp

+				Ort::Value value = Ort::Value::CreateTensor(
+					allocator, shape.data(), shape.size(), ONNX_TENSOR_ELEMENT_DATA_TYPE_STRING
+				);
+				std::copy( typedData->readable().begin(), typedData->readable().end(), value.GetTensorMutableData<std::string>() );


I'm really surprised this works. As far as I can see, the underlying ONNX C library represents strings as const char *, and there's no guarantee that std::string has the same layout (although it is possible to implement it so). Even if it did have the same layout, I think we've leaked the strings here, because the destructors for the copies will never get called (there aren't std::string objects in the C library).

I think we really need to build a vector<const char *> here and pass it to value.FillStringTensor() instead. Does that make sense?

Yes that makes sense but this comment is very confusing https://github.com/microsoft/onnxruntime/blob/main/include/onnxruntime/core/session/onnxruntime_c_api.h#L186

addressed here c7db62d

Yes that makes sense but this comment is very confusing

Oh, that is confusing! Looking at the source it looks like the C library is actually implemented in C++ behind the scenes, so it's plausible that the tensor is backed by std::string, which would also explain how this was working before. I'd prefer not to depend on that though and go through the "proper" API as you've done now - thanks for the update.

johnhaddon · 2025-02-11T10:26:46Z

src/GafferML/Tensor.cpp

@@ -125,6 +125,9 @@ void dispatchTensorData( const Ort::Value &value, F &&functor )
 		case ONNX_TENSOR_ELEMENT_DATA_TYPE_INT64 :
 			functor( value.GetTensorData<int64_t>() );
 			break;
+		case ONNX_TENSOR_ELEMENT_DATA_TYPE_STRING :
+			functor( value.GetTensorData<std::string>() );


I'm surprised that this works - as I understand it, there are const char * elements in the tensor, so casting them to std::string will likely blow up. Would be good to have some unit tests to demonstrate this all working. We also need to update tensorGetItem() in GafferMLModule.cpp to handle strings, which will help with the testing.

I would need a bit more information here because it looks to me that the generic functor wont work here, right?
We need something a bit more specific to reconstruct std::string from char*, right?
Or do we convert into some kind of CharVectorData?

Ugh, yeah, this is messy. The ONNX docs do say we can't do it this way though - GetTensorData calls through to the C GetTensorMutableData function where the docs say this :

\param[in] value A tensor type (string tensors are not supported)

We don't really want to convert to another data type either though - for most of the uses of dispatchTensorData() it would be wasteful to make a copy. Even if we are getting lucky here and getting a pointer to real std::strings , Tensor::isEqualTo() and Tensor::memoryUsage() look broken since they're assuming they're looking at POD types, and won't handle the fact that the string contains a pointer. Since 50% of the clients need a special case anyway, we could argue that dispatchTensorData() should throw for string data, forcing clients to think carefully and implement a special case.

All this just convinces me further of the need for really strong testing of the string tensors .

src/GafferML/Inference.cpp

johnhaddon · 2025-02-14T14:13:41Z

Thanks for the update Lucien, and for pointing out the mysterious std::string references in the C documentation. If indeed the tensors are backed by std::string, it is a little tempting to rely on that if it makes thing easier in the short term. But I'd only be willing to do that if we have robust test coverage for all string tensor operations - equality, copy, hash, indexing, conversion to data etc. I think getting that coverage in place is the key - it would let us confirm the possibility of there being real strings there, and give us a safety net if we decide to proceed on that assumption.

Add support for string tensors

2d385e0

lucienfostier changed the base branch from main to 1.5_maintenance February 8, 2025 19:58

Inference.cpp: Add support for registering custom op library.

260f7f3

This can be useful for models like Grounding Dino that requires the onnx runtime extension for the BERT tokenizer.

lucienfostier force-pushed the groundingDinoRequirements branch from 8c1d9db to 260f7f3 Compare February 8, 2025 19:59

johnhaddon reviewed Feb 11, 2025

View reviewed changes

lucienfostier added 2 commits February 11, 2025 21:03

fixup! Add support for string tensors

c7db62d

fixup! Inference.cpp: Add support for registering custom op library.

6f5be1b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grounding dino requirements #6263

Grounding dino requirements #6263

lucienfostier commented Feb 8, 2025

lucienfostier commented Feb 8, 2025

johnhaddon left a comment

johnhaddon Feb 11, 2025

lucienfostier Feb 12, 2025

johnhaddon Feb 11, 2025

lucienfostier Feb 12, 2025

lucienfostier Feb 12, 2025

johnhaddon Feb 14, 2025

johnhaddon Feb 11, 2025

lucienfostier Feb 12, 2025

johnhaddon Feb 14, 2025

johnhaddon commented Feb 14, 2025

Grounding dino requirements #6263

Are you sure you want to change the base?

Grounding dino requirements #6263

Conversation

lucienfostier commented Feb 8, 2025

Checklist

lucienfostier commented Feb 8, 2025

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhaddon commented Feb 14, 2025