DATA-4320 - Implement BinaryDataToDataset in datamanager in RDK #5145

tahiyasalam · 2025-07-16T18:54:48Z

Overview

This PR implements the new endpoint UploadBinaryDataToDataset as part of the DataManager Service. We also implement both UploadBinaryDataToDataset and UploadImageToDataset as part of the Go SDK, where the latter is just a convenience method that converts an image.Image into bytes before calling the same underlying upload binary data method.

Scope: https://docs.google.com/document/d/1YSJ3lTz5sC5HP5xguvd0VqSAQNoUVPSWrvj3zldW0Uo/edit?tab=t.0#heading=h.tcicyojyqi6c

Testing

Added tests in builtin_sync_test.go, looking for feedback in where else to add tests
Manual testing within a module ✅

…data for datasets file

etai-shuchatowitz · 2025-07-18T14:56:41Z

go.mod

@@ -444,3 +444,5 @@ require (
 	github.com/ziutek/mymysql v1.5.4 // indirect
 	golang.org/x/exp v0.0.0-20240904232852-e7e105dedf7e
 )
+
+replace go.viam.com/api => github.com/viamrobotics/api v0.1.458-0.20250717192712-d9437d8203b6 // upload-to-dataset


Just a reminder that we’ll have to update the API proper before merging

etai-shuchatowitz · 2025-07-18T14:58:29Z

services/datamanager/builtin/builtin.go

@@ -259,3 +260,34 @@ func lookupCollectorConfigsByResource(
 	}
 	return collectorConfigsByResource, nil
 }
+
+func (b *builtIn) UploadBinaryDataToDataset(ctx context.Context,
+	image []byte,


Can we rename this binary_data?

etai-shuchatowitz · 2025-07-18T15:01:38Z

services/datamanager/builtin/builtin_sync_test.go

+				fileContents = append(fileContents, datasetBytes...)
+				timeoutCtx, timeoutFn := context.WithTimeout(context.Background(), time.Second*5)
+				defer timeoutFn()
+				for {


Sorry — not quite understanding why this is in a retry loop.

Are we expecting UploadBinaryDataToDataset to potentially fail and need to be retried?

I'm actually not sure, was mainly following the structure above for Syncing

etai-shuchatowitz · 2025-07-18T15:02:38Z

services/datamanager/builtin/builtin_sync_test.go

 			if !tc.serviceFail {
 				// Validate first metadata message.
-				test.That(t, uploadCount.Load(), test.ShouldEqual, 1)
+				expectedUploadedCount := 1


Can we make this a variable that runs per test. Meaning, TestA has 3 expectedUploadedCount, TestB has 2...

I think the test relies on conditionals and checking only the expected count within this condition, so I'm keen to leave as is as to not introduce an even larger diff if that's ok

services/datamanager/builtin/builtin_sync_test.go

etai-shuchatowitz · 2025-07-18T15:07:22Z

services/datamanager/builtin/sync/sync.go

@@ -476,6 +480,35 @@ func (s *Sync) syncArbitraryFile(f *os.File, tags []string, fileLastModifiedMill
 	s.atomicUploadStats.arbitrary.uploadedBytes.Add(bytesUploaded)
 }

+// UploadBinaryDataToDataset simultaneously uploads binary data and adds it to a dataset.
+func (s *Sync) UploadBinaryDataToDataset(ctx context.Context, data []byte, datasetIDs, tags []string, mimeType v1.MimeType) error {


I think the ask was for the writing to file to happen async as well? Like, to wrap all of this into its own goroutine.

etai-shuchatowitz · 2025-07-18T15:08:43Z

services/datamanager/client.go

+	mimeType datasyncpb.MimeType,
+	extra map[string]interface{},
+) error {
+	ext, err := protoutils.StructToStructPb(extra)


Nit: can we change this to extra. Just we also have the word extension being thrown around and that’s shortened to ext

katiepeters · 2025-07-18T19:39:35Z

services/datamanager/builtin/builtin.go

@@ -259,3 +260,34 @@ func lookupCollectorConfigsByResource(
 	}
 	return collectorConfigsByResource, nil
 }
+
+func (b *builtIn) UploadBinaryDataToDataset(ctx context.Context,


Suggested change

func (b *builtIn) UploadBinaryDataToDataset(ctx context.Context,

func (b *builtIn) UploadBinaryDataToDatasets(ctx context.Context,

Although I guess it looks like the name was already solidified?

ended up changing this, thank you!

katiepeters · 2025-07-18T19:42:20Z

services/datamanager/builtin/builtin.go

+		return err
+	}
+	return b.sync.UploadBinaryDataToDataset(ctx, imgBytes, datasetIDs, tags, mimeType)
+}


Are errors from these new functions all safe to surface to the user?

Yes, I believe these errors should be safe to surface to the user

katiepeters · 2025-07-18T19:48:57Z

services/datamanager/builtin/builtin_sync_test.go

+				fileContents = append(fileContents, datasetBytes...)
+				timeoutCtx, timeoutFn := context.WithTimeout(context.Background(), time.Second*5)
+				defer timeoutFn()
+				for {


Are we expecting UploadBinaryDataToDataset to potentially fail and need to be retried?

katiepeters · 2025-07-18T19:59:49Z

services/datamanager/builtin/sync/sync.go

-			if info.IsDir() && info.Name() == FailedDir {
+			// Do not sync the files in the corrupted data directory or in the directory that holds files
+			// that are simultaneously uploaded and added to a dataset.
+			if info.IsDir() && (info.Name() == FailedDir || info.Name() == DatasetDir) {


I'd be more comfortable if the skipped directory had a name that was very unlikely to be used by users. "dataset" doesn't feel out of the realm of possibility 🤔

This directory would be .viam/capture/dataset not just the directory dataset. I can change it to viamUploadToDataset or something? Open to suggestions

katiepeters · 2025-07-18T20:05:51Z

services/datamanager/client.go

+
+func (c *client) UploadBinaryDataToDataset(
+	ctx context.Context,
+	image []byte,


Suggested change

image []byte,

binaryData []byte,

katiepeters · 2025-07-18T20:09:30Z

services/datamanager/client.go

+}
+
+// ConvertImageToBytes converts an image.Image to a byte slice based on the specified MIME type.
+func ConvertImageToBytes(image image.Image, mimeType datasyncpb.MimeType) ([]byte, error) {


Question: Do we have concerns about loading the entire image into memory?

I don't think so, though I can add this as a follow up. This is a paradigm that's repeated throughout RDK

katiepeters · 2025-07-18T20:11:58Z

services/datamanager/server.go

@@ -34,6 +34,21 @@ func (server *serviceServer) Sync(ctx context.Context, req *pb.SyncRequest) (*pb
 	return &pb.SyncResponse{}, nil
 }

+func (server *serviceServer) UploadImageToDataset(


Is the naming of this confusing?

updated, was a typo

katiepeters · 2025-07-18T20:15:09Z

services/datamanager/builtin/builtin.go

+	binaryData []byte,
+	datasetIDs, tags []string,
+	mimeType v1.MimeType,
+	extra map[string]interface{},


Doesn't look like extra is used?

katiepeters · 2025-07-18T20:17:20Z

Did a quick pass, but I'm going to add @vijayvuyyuru because I'm going to be mostly OOO next week.

go.mod

vijayvuyyuru · 2025-07-21T19:12:14Z

services/datamanager/builtin/builtin.go

+	mimeType v1.MimeType,
+	_ map[string]interface{},
+) error {
+	b.logger.Info("UploadBinaryDataToDatasets START")


I think these should be debug level logs? Seems excessive on every upload call.

vijayvuyyuru · 2025-07-21T19:13:13Z

services/datamanager/builtin/builtin.go

+	mimeType v1.MimeType,
+	_ map[string]interface{},
+) error {
+	b.logger.Info("UploadImageToDataset START")


vijayvuyyuru · 2025-07-21T19:14:01Z

services/datamanager/builtin/builtin.go

+) error {
+	b.logger.Info("UploadImageToDataset START")
+	defer b.logger.Info("UploadImageToDataset END")
+	b.mu.Lock()


I feel like this locking should only be around the upload itself? I feel like safe to leave unlocked if we haven't converted the image.

vijayvuyyuru · 2025-07-21T19:17:36Z

services/datamanager/builtin/builtin.go

+	binaryData []byte,
+	datasetIDs, tags []string,
+	mimeType v1.MimeType,
+	_ map[string]interface{},


I think these should be passed through as part of the Extra field. Same for UploadBinaryImage

This method calls the sync client's UploadBinaryDataToDatasets and that does not take an extra

vijayvuyyuru · 2025-07-21T19:44:42Z

services/datamanager/builtin/sync/sync.go

+	go func() {
+		defer close(errChan)
+		// Create a new directory CaptureDir/DatasetDir
+		newDir := filepath.Join(s.config.CaptureDir, DatasetDir)


[q] Feel free to ignore this. But I feel like this logic could be moved to reconfigure? Like making the directory as that unformation only changes then as opposed to trying to do it on each upload call?

I think we would only want to create this directory if we were actively trying to make the upload call. If the directory already exists, this should just be a no-op

vijayvuyyuru · 2025-07-21T19:57:05Z

services/datamanager/client.go

+		return err
+	}
+
+	imgBytes, err := ConvertImageToBytes(image, mimeType)


[nit] Could we change this function to just call c.UploadBinaryDataToDataSets? That way we dont need the marshalling of extra from map -> structpb defined twice

WIP - Ability to add image to dataset in datamanager

c26cc8a

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jul 16, 2025

Add mime type to upload binary data and change filename

da48543

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 17, 2025

Add logic for dataset directory; TODO - add tests

92925bc

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 17, 2025

WIP - Adding tests to arbitrary file upload; TODO - check upload meta…

ccdee28

…data for datasets file

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 17, 2025

Add checks for dataset metadata in arbitrary file upload

36bcfdc

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 17, 2025

Temporarily replacing api with PR branch

a9b3ebc

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 17, 2025

tahiyasalam changed the title ~~WIP - Ability to add image to dataset in datamanager~~ DATA-4320 - Implement BinaryDataToDataset in datamanager in RDK Jul 17, 2025

Lint

89b77b7

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 18, 2025

tahiyasalam requested review from etai-shuchatowitz and katiepeters July 18, 2025 13:01

etai-shuchatowitz reviewed Jul 18, 2025

View reviewed changes

services/datamanager/builtin/builtin_sync_test.go Show resolved Hide resolved

etai-shuchatowitz reviewed Jul 18, 2025

View reviewed changes

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 18, 2025

Add go routine stuff

5e9604a

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 18, 2025

katiepeters reviewed Jul 18, 2025

View reviewed changes

katiepeters requested a review from vijayvuyyuru July 18, 2025 20:17

Remove extra

1ef43fc

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 21, 2025

Change from dataset > datasets

10b8588

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 21, 2025

Fix response

405089a

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 21, 2025

vijayvuyyuru reviewed Jul 21, 2025

View reviewed changes

PR comments

3cb8ed0

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 24, 2025

Add to Go SDK

0a552d3

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 29, 2025

Unify naming

31a51cd

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 31, 2025

tahiyasalam requested review from katiepeters, vijayvuyyuru and etai-shuchatowitz July 31, 2025 20:33

	func (b *builtIn) UploadBinaryDataToDataset(ctx context.Context,
	func (b *builtIn) UploadBinaryDataToDatasets(ctx context.Context,

DATA-4320 - Implement BinaryDataToDataset in datamanager in RDK #5145

Are you sure you want to change the base?

DATA-4320 - Implement BinaryDataToDataset in datamanager in RDK #5145

Uh oh!

Conversation

tahiyasalam commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katiepeters commented Jul 18, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tahiyasalam commented Jul 16, 2025 •

edited

Loading