[DATA-645] Make CLI download binary in parallel. #1659

AaronCasas · 2022-12-12T12:10:08Z

Adds a concurrentDownloads parameter that controls the number of download binary requests to make in parallel.

Tested locally and:

Achieved ~N/2 times speed up, where N is the value of concurrentDownloads. With a value of 25, reduced download time for 2000 images from 5m30s -> 25s. For 11,000 images, reduced the time from a timeout -> 1m30s. Can test locally by building this branch then running go run go.viam.com/rdk/cli/cmd data --org_ids=1c614556-2ff9-4234-9a94-d59b0a6d3378 --data_type=binary --mime_types=image/jpeg,image/png --start=2022-11-01T04:00:00.000Z --destination=/tmp/cli_speed_test/1 --concurrent=25. I suspect that we'd approach N times speed up as the amount of total data downloaded increases.
Confirmed that receiving an error causes all other requests to be cancelled and the cli to return

Something to note is that rpi images are very small (~16kb it seems). For files this small, having one goroutine per download is pretty inefficient. A future optimization would be to more adjust the number of downloads per routine based on the size of each file (which we can grab from the metadata when grabbing the id). For the time being this seems sufficient, though, especially since this sub-optimal behavior is most relevant in "small" cases where optimal performance is least important (because it will still be pretty damn fast). If we discover that exporting very large numbers of very small is a common use case, we can prioritize this.

…context.Cancelled from cancelling other things

…p-cli

github-actions · 2022-12-12T16:25:30Z

Package	Line Rate	Delta	Health
go.viam.com/rdk/components/arm	61%	0.00%	➖
go.viam.com/rdk/components/arm/universalrobots	13%	0.00%	❌
go.viam.com/rdk/components/arm/xarm	2%	0.00%	❌
go.viam.com/rdk/components/arm/yahboom	7%	0.00%	❌
go.viam.com/rdk/components/audioinput	55%	+0.34%	➖
go.viam.com/rdk/components/base	67%	0.00%	➖
go.viam.com/rdk/components/base/agilex	62%	0.00%	➖
go.viam.com/rdk/components/base/boat	41%	0.00%	❌
go.viam.com/rdk/components/base/wheeled	76%	0.00%	✅
go.viam.com/rdk/components/board	68%	0.00%	➖
go.viam.com/rdk/components/board/arduino	10%	0.00%	❌
go.viam.com/rdk/components/board/commonsysfs	47%	0.00%	❌
go.viam.com/rdk/components/board/fake	38%	0.00%	❌
go.viam.com/rdk/components/board/numato	19%	0.00%	❌
go.viam.com/rdk/components/board/pi	50%	0.00%	➖
go.viam.com/rdk/components/camera	65%	0.00%	➖
go.viam.com/rdk/components/camera/fake	67%	0.00%	➖
go.viam.com/rdk/components/camera/ffmpeg	77%	+1.43%	✅
go.viam.com/rdk/components/camera/transformpipeline	78%	+0.33%	✅
go.viam.com/rdk/components/camera/videosource	56%	+0.51%	➖
go.viam.com/rdk/components/encoder/fake	77%	0.00%	✅
go.viam.com/rdk/components/gantry	68%	0.00%	➖
go.viam.com/rdk/components/gantry/multiaxis	84%	0.00%	✅
go.viam.com/rdk/components/gantry/oneaxis	86%	0.00%	✅
go.viam.com/rdk/components/generic	83%	0.00%	✅
go.viam.com/rdk/components/gripper	80%	0.00%	✅
go.viam.com/rdk/components/input	87%	0.00%	✅
go.viam.com/rdk/components/input/gpio	84%	0.00%	✅
go.viam.com/rdk/components/motor	81%	0.00%	✅
go.viam.com/rdk/components/motor/dimensionengineering	63%	0.00%	➖
go.viam.com/rdk/components/motor/dmc4000	69%	0.00%	➖
go.viam.com/rdk/components/motor/fake	57%	0.00%	➖
go.viam.com/rdk/components/motor/gpio	64%	+2.16%	➖
go.viam.com/rdk/components/motor/gpiostepper	57%	0.00%	➖
go.viam.com/rdk/components/motor/tmcstepper	62%	0.00%	➖
go.viam.com/rdk/components/movementsensor	75%	0.00%	✅
go.viam.com/rdk/components/movementsensor/cameramono	40%	0.00%	❌
go.viam.com/rdk/components/movementsensor/gpsnmea	37%	0.00%	❌
go.viam.com/rdk/components/movementsensor/gpsrtk	28%	0.00%	❌
go.viam.com/rdk/components/posetracker	86%	0.00%	✅
go.viam.com/rdk/components/sensor	86%	0.00%	✅
go.viam.com/rdk/components/sensor/ultrasonic	34%	0.00%	❌
go.viam.com/rdk/components/servo	78%	0.00%	✅
go.viam.com/rdk/components/servo/gpio	71%	0.00%	✅
go.viam.com/rdk/config	77%	+0.07%	✅
go.viam.com/rdk/control	57%	0.00%	➖
go.viam.com/rdk/data	77%	0.00%	✅
go.viam.com/rdk/grpc	25%	0.00%	❌
go.viam.com/rdk/ml	67%	0.00%	➖
go.viam.com/rdk/ml/inference	70%	0.00%	✅
go.viam.com/rdk/motionplan	71%	+2.74%	✅
go.viam.com/rdk/octree	93%	0.00%	✅
go.viam.com/rdk/operation	82%	0.00%	✅
go.viam.com/rdk/pointcloud	71%	0.00%	✅
go.viam.com/rdk/protoutils	59%	0.00%	➖
go.viam.com/rdk/referenceframe	71%	0.00%	✅
go.viam.com/rdk/registry	90%	0.00%	✅
go.viam.com/rdk/resource	83%	0.00%	✅
go.viam.com/rdk/rimage	78%	0.00%	✅
go.viam.com/rdk/rimage/depthadapter	94%	0.00%	✅
go.viam.com/rdk/rimage/transform	73%	0.00%	✅
go.viam.com/rdk/rimage/transform/cmd/extrinsic_calibration	67%	0.00%	➖
go.viam.com/rdk/robot	85%	0.00%	✅
go.viam.com/rdk/robot/client	82%	0.00%	✅
go.viam.com/rdk/robot/framesystem	68%	0.00%	➖
go.viam.com/rdk/robot/impl	81%	0.00%	✅
go.viam.com/rdk/robot/server	57%	0.00%	➖
go.viam.com/rdk/robot/web	60%	0.00%	➖
go.viam.com/rdk/robot/web/stream	87%	0.00%	✅
go.viam.com/rdk/services/armremotecontrol	71%	0.00%	✅
go.viam.com/rdk/services/armremotecontrol/builtin	52%	0.00%	➖
go.viam.com/rdk/services/baseremotecontrol	71%	0.00%	✅
go.viam.com/rdk/services/baseremotecontrol/builtin	79%	0.00%	✅
go.viam.com/rdk/services/datamanager	65%	0.00%	➖
go.viam.com/rdk/services/datamanager/builtin	80%	0.00%	✅
go.viam.com/rdk/services/datamanager/datacapture	21%	0.00%	❌
go.viam.com/rdk/services/datamanager/datasync	72%	0.00%	✅
go.viam.com/rdk/services/motion	63%	0.00%	➖
go.viam.com/rdk/services/motion/builtin	88%	0.00%	✅
go.viam.com/rdk/services/navigation	54%	0.00%	➖
go.viam.com/rdk/services/sensors	77%	0.00%	✅
go.viam.com/rdk/services/sensors/builtin	97%	0.00%	✅
go.viam.com/rdk/services/shell	14%	0.00%	❌
go.viam.com/rdk/services/slam	84%	0.00%	✅
go.viam.com/rdk/services/slam/builtin	72%	-0.51%	✅
go.viam.com/rdk/services/vision	80%	0.00%	✅
go.viam.com/rdk/services/vision/builtin	74%	0.00%	✅
go.viam.com/rdk/session	97%	0.00%	✅
go.viam.com/rdk/spatialmath	85%	0.00%	✅
go.viam.com/rdk/subtype	96%	0.00%	✅
go.viam.com/rdk/utils	72%	+0.18%	✅
go.viam.com/rdk/vision	26%	0.00%	❌
go.viam.com/rdk/vision/chess	80%	0.00%	✅
go.viam.com/rdk/vision/delaunay	87%	0.00%	✅
go.viam.com/rdk/vision/keypoints	92%	0.00%	✅
go.viam.com/rdk/vision/objectdetection	82%	0.00%	✅
go.viam.com/rdk/vision/odometry	60%	0.00%	➖
go.viam.com/rdk/vision/odometry/cmd	0%	0.00%	❌
go.viam.com/rdk/vision/segmentation	49%	0.00%	❌
go.viam.com/rdk/web/server	26%	0.00%	❌
Summary	67% (20440 / 30645)	+0.23%	➖

agreenb

Really awesome and quick work! Excited about this massive speedup — thanks for the thorough writeup, clean code, and stress/performance testing! Mostly minor comments.

agreenb · 2022-12-12T22:40:32Z

cli/cmd/main.go

+	dataFlagMimeTypes           = "mime_types"
+	dataFlagStart               = "start"
+	dataFlagEnd                 = "end"
+	dataFlagConcurrentDownloads = "concurrent"


[optional nit] Even though concurrency and parallelism are different, I think to the user "parallel" is the more common user-friendly term, so would potentially make this --parallel and explain it's concurrent in the usage description.

Yeah parallel is definitely the more accurate word here, updated

agreenb · 2022-12-12T22:45:14Z

cli/data.go

+	go func() {
+		defer wg.Done()
+		var limit int
+		if concurrentDownloads > 100 {


[nit] Extract this into a const at the top of the file

[optional] If we bump up to the latest stable version of urfave/cli, we can use flag validation to only accept a value within a certain range: https://github.com/urfave/cli/blob/main/docs/v2/examples/flags.md#flag-actions

I think we still want to support parallel values higher than this limit, so I'm going to still allow arbitrary values for parallel. This will just get the IDs in multiple requests, but the channel feeding them into the download routine is still parallelRequests big, so we can still do parallelRequests at a time

Added maxLimit const

That's a really clever way to limit the server load from BinaryDataByIDs calls but parallelize the actual local downloads further. Nice.

agreenb · 2022-12-12T23:03:34Z

cli/cmd/main.go

+					&cli.IntFlag{
+						Name:     dataFlagConcurrentDownloads,
+						Required: false,
+						Usage:    "number of download requests to make in parallel",


Would make a note to explicitly set this value to 1 for consecutive downloads, since we otherwise default to 10 concurrent requests.

Added not about default value but left out the note about needing to set to 1 for non-parallel downloads, because I think it's directly implied

agreenb · 2022-12-12T23:07:27Z

cli/data.go

+				}(nextID)
+			}
+			downloadWG.Wait()
+			if numFilesDownloaded.Load()%logEveryN == 0 {


I believe this would only print every N if concurrentDownloads % logEveryN == 0. Can move this right after numFilesDownload.Add(1) above or, if that somehow runs into concurrency oddness, set logEveryN based on numFilesDownloaded.

Ah yup you're totally right, added to within the download goroutine so this check is done on every download

agreenb · 2022-12-12T23:13:31Z

cli/data.go

+	}
+
+	//nolint:gosec
+	dataFile, err := os.Create(filepath.Join(dst, "data", datum.GetMetadata().GetId()+datum.GetMetadata().GetFileExt()))


Looks like this section also lost the newest changes with dataDir/metadataDir and prepending timestamp to the filename. The latter is especially needed now that we're interleaving downloads :)

If you could locally test a scenario where you're moving a motor while collecting data and, post-sort, seeing all the data in chronological order, that'd be great.

In the future when we support downloads via our cloud service, we can actually sort + package nicely before downloading to browser.

Oops yup bad merging, added the naming stuff back.

If you could locally test a scenario where you're moving a motor while collecting data and, post-sort, seeing all the data in chronological order, that'd be great.

It looks like we weren't sorting tabular data by timestamp before, and this doesn't do tabular export in parallel. Did you mean for binary?

While exporting tabular I do think I found a bug in how we're persisting time stamps for tabular data, though. For any syncInterval, it looks like we record the same time requested/received for all data points. You can see this by running this command:
go run go.viam.com/rdk/cli/cmd data --location_ids=zqegrpxfhl --robot_name=Detector --org_ids=1c614556-2ff9-4234-9a94-d59b0a6d3378 --data_type=tabular --start=2022-11-01T04:00:00.000Z

This data was being collected at 10hz with a sync interval of 1 minute. If you open the file, you can see every 600 messages have the same time requested/received. I think we should prioritize fixing this.

Ah yup found the bug, will tag you on follow up PR

Fix out here https://github.com/viamrobotics/app/pull/962/files

It looks like we weren't sorting tabular data by timestamp before, and this doesn't do tabular export in parallel. Did you mean for binary?

Yep meant to add this in the binary section, and both tabular/binary needed merge updates.

This data was being collected at 10hz with a sync interval of 1 minute. If you open the file, you can see every 600 messages have the same time requested/received. I think we should prioritize fixing this.

Really cool seeing the E2E manual testing uncovering a bug like this. Fantastic job on both for spotting it and pushing out a super quick fix!

Interesting to see that even though it had full test coverage, the tests were expecting the wrong thing. Worth listing out scenarios in PR descriptions to help get high-level consensus on test behavior, since sometimes harder to spot when writing/reviewing as code.

agreenb · 2022-12-12T23:14:41Z

cli/data.go

+	if err := r.Close(); err != nil {
+		return err
+	}
+	return nil
 }

 // TabularData downloads binary data matching filter to dst.


Looks like TabularData needs to sync to the newer changes with maxRetryCount, logging, and dataDir/metadataDir

Oof yup more bad merging on my part, fixed

agreenb · 2022-12-12T23:29:29Z

cli/data.go

+				Limit:  uint64(limit),
+				Last:   last,
+			},
+			CountOnly: false,


Can also explicitly set IncludeBinary to false here.

tahiyasalam · 2022-12-13T14:54:03Z

cli/data.go

-		datum := data[0]
-		mdJSONBytes, err := protojson.Marshal(datum.GetMetadata())
-		if err != nil {
+// getMatchingIDs queries client for all BinaryData matching filter, and passes each of their ids into ids.


supernit: "// getMatchingIDs queries client for all BinaryData matching filter and passes each of their ids into channel."

tahiyasalam · 2022-12-13T14:54:20Z

cli/data.go

+				nextID = <-ids
+
+				// If nextID is zero value, the channel has been closed and there are no more IDs to be read.
+				if nextID == "" {


supernit: If nextID is empty

tahiyasalam · 2022-12-13T14:55:24Z

cli/data.go

 		if err != nil {
 			return err
 		}
-		if _, err := jsonFile.Write(mdJSONBytes); err != nil {
-			return err
+		// If no data is returned, there is no more data.


supernit/opt: comment seems redundant, maybe just "No more data is returned."?

tahiyasalam

Super cool! A couple nit things, but I ran this locally and was very excited by the results!

tahiyasalam · 2022-12-13T15:25:19Z

cli/data.go

-		if err != nil {
-			return errors.Wrapf(err, "received error from server")
+		if numFilesDownloaded.Load()%logEveryN != 0 {
+			fmt.Fprintf(c.c.App.Writer, "downloaded %d files to %s\n", numFilesDownloaded.Load(), dst)


nit: I find it helpful as a user to have (downloaded # files / total files) as a indication of what's done and what's still left to go

agreenb

Immeasurably excited about this change. Really amazing work. 💯 🚀

agreenb · 2022-12-13T17:51:11Z

cli/data.go

@@ -202,7 +208,7 @@ func downloadBinary(ctx context.Context, client datapb.DataServiceClient, dst, i
 	}

 	//nolint:gosec
-	dataFile, err := os.Create(filepath.Join(dst, "data", datum.GetMetadata().GetId()+datum.GetMetadata().GetFileExt()))
+	dataFile, err := os.Create(filepath.Join(dst, dataDir, "data"+".ndjson"))


Think this got copied from tabular, but this would instead be dataFile, err := os.Create(filepath.Join(dst, dataDir, fileName+datum.GetMetadata().GetFileExt())) in order to reuse the timestamped binary file and its file extension

Eek yes bad copying/pasting, fixed

agreenb · 2022-12-13T17:54:21Z

cli/data.go

+			DataRequest: &datapb.DataRequest{
+				Filter: filter,
+				// TODO: For now don't worry about skip/limit. Just do everything in one request. Can implement batching when
+				//       tabular is implemented.


Checking on this comment which was copied over from before: is this referencing if a TabularDataByIDs is implemented or something else?

It was actually referencing when tabular wasn't implemented in the backend yet. So right now this is actually just trying to get all the sensor data in a single request. This is fine for small values (< default max proto message size, which I believe is like 4MB), but won't work for larger ones. I made a ticket to add this and linked in it the TODO.

agreenb · 2022-12-13T17:56:03Z

cli/cmd/main.go

+					&cli.IntFlag{
+						Name:     dataFlagConcurrentDownloads,
+						Required: false,
+						Usage:    "number of download requests to make in parallel",


agreenb · 2022-12-13T17:57:12Z

cli/data.go

+	go func() {
+		defer wg.Done()
+		var limit int
+		if concurrentDownloads > 100 {


That's a really clever way to limit the server load from BinaryDataByIDs calls but parallelize the actual local downloads further. Nice.

tessavitabile · 2022-12-13T18:55:18Z

cli/data.go

-		for count := 0; count < maxRetryCount; count++ {
-			resp, err = c.sendBinaryDataByFilterRequest(filter, last)
-			if err == nil {
+	if parallelDownloads == 0 {


Do we need to handle a negative value?

Great call out, I'll go ahead and change it to a uint flag to validate that from the start

Make CLI do 10 download binary requests at a time concurrently.

5dc4dd9

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Dec 12, 2022

Cancel goroutines if one fails.

74b16a1

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 Dec 12, 2022

Make number of concurrent downloads a param.

5b615f1

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 Dec 12, 2022

Ensure real error is returned when we receive an error, not just the …

9ec8891

…context.Cancelled from cancelling other things

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 Dec 12, 2022

AaronCasas changed the title ~~[DATA-645] Make CLI do 10 download binary requests at a time concurrently.~~ [DATA-645] Make CLI download binary in parallel. Dec 12, 2022

AaronCasas requested review from agreenb and tahiyasalam December 12, 2022 14:28

Don't need if for getting dataFlagConcurrent.

051c5b6

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 Dec 12, 2022

Add progress logging back.

28ac99c

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 Dec 12, 2022

Merge branch 'main' into speed-up-cli

3dac5e4

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 Dec 12, 2022

aaroncasas added 2 commits December 12, 2022 10:08

We don't need a special case for zero.

27af955

Merge branch 'speed-up-cli' of github.com:AaronCasas/rdk into speed-u…

a3d2c53

…p-cli

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 Dec 12, 2022

AaronCasas marked this pull request as ready for review December 12, 2022 15:14

Huh

c3d334e

viambot removed the safe to test This pull request is marked safe to test from a trusted zone label Dec 12, 2022

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Dec 12, 2022

AaronCasas requested a review from tessavitabile December 12, 2022 19:20

Give limit a max size.

30cb160

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 Dec 12, 2022

agreenb suggested changes Dec 12, 2022

View reviewed changes

Explanatory comment for magic limit limit.

57d6d67

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 Dec 13, 2022

tahiyasalam reviewed Dec 13, 2022

View reviewed changes

PR commments.

c91851e

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 Dec 13, 2022

agreenb approved these changes Dec 13, 2022

View reviewed changes

tessavitabile reviewed Dec 13, 2022

View reviewed changes

Uint flag for parallel; add data-928 todo.

bbf0d04

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 Dec 13, 2022

agreenb approved these changes Dec 13, 2022

View reviewed changes

AaronCasas merged commit 382da58 into viamrobotics:main Dec 13, 2022

AaronCasas deleted the speed-up-cli branch December 13, 2022 20:40

bazile-clyde mentioned this pull request Feb 7, 2023

RSDK 1845 bazile-clyde/rdk#1

Closed

[DATA-645] Make CLI download binary in parallel. #1659

[DATA-645] Make CLI download binary in parallel. #1659

Conversation

AaronCasas commented Dec 12, 2022 • edited Loading

github-actions bot commented Dec 12, 2022

agreenb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronCasas Dec 13, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

tahiyasalam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agreenb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronCasas Dec 13, 2022 • edited Loading

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

AaronCasas commented Dec 12, 2022 •

edited

Loading

AaronCasas Dec 13, 2022 •

edited

Loading

AaronCasas Dec 13, 2022 •

edited

Loading