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

[DATA-3338] fix-stability-of-vision-capture-all-from-camera-and-camera-get-images #4557

Conversation

nicksanford
Copy link
Member

@nicksanford nicksanford commented Nov 15, 2024

Why:

  1. the collectors for vision.CaptureAllFromCamera and camera.GetImages write the responses from those methods as tabular data. That results a go object being allocated for every pixle of the images in those method's responses. At high capture rates that results in heap fragmentation and viam-server crashing due to running out of memory. See https://viam.atlassian.net/browse/DATA-3191 for the tests that show that.
  2. To fix that problem we need all binary collectors to write binary results. This requires allowing a binary capture file to contain multiple binary results (as that is what camera.GetImages returns).
  3. We also needed to write annotations to capture files as that is what vision.CaptureAllFromCamera needs to describe what the ML model detected in the image.

Fixes:

Changes:

  1. Change signature of collectors from (ctx context.Context, _ map[string]*anypb.Any) (interface{}, error) to (ctx context.Context, _ map[string]*anypb.Any) (data.CaptureResult, error) see data/collector_types.go for more info
  2. Change CaptureFuncs to determine the capture request and response time rather than data/collector.go
  3. Change all tabular collectors to use data.NewTabularCaptureResult to create tabular capture results.
  4. Update collector tests to assert on multiple sensor data values
  5. Update data sync to accept binary capture files with multiple readings (as in the case of camera.GetImages)
  6. Update data sync to handle both legacy camera.GetImages capture files and the new, more efficient ones
  7. Update data sync to set both the datasyncpb.MimeType and the datasyncpb.UploadMetadata.FileExtension fields
  8. Update data sync to move capture file to failed directory in cases where the capture file contains unrecoverable data (such as multiple capture types within a given capture file, or the capture file header type being something other than binary or tabular)

@viambot 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 Nov 15, 2024
@nicksanford nicksanford force-pushed the DATA-3338-fix-stability-of-vision-capture-all-from-camera-2 branch from 33c6fab to 501209a Compare November 18, 2024 18:04
@viambot 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 Nov 18, 2024
@viambot 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 Nov 19, 2024
@nicksanford nicksanford force-pushed the DATA-3338-fix-stability-of-vision-capture-all-from-camera-2 branch from 602c8aa to 588a56e Compare December 11, 2024 18:14
@viambot 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 11, 2024
Copy link
Member

@dmhilly dmhilly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work on this Nick 🎉🎉

@viambot 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 11, 2024
data/collector_types.go Outdated Show resolved Hide resolved
Co-authored-by: Sagie Maoz <sagiem@gmail.com>
@viambot 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, 2024
Copy link
Member

@n0nick n0nick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines 45 to 46
// expectedUploadMetadata []*v1.UploadMetadata
// expectedSensorData [][]*v1.SensorData
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

partID := "my-part-id"

now := time.Now()
ts := data.Timestamps{TimeRequested: now, TimeReceived: time.Now()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should you use now instead of calling time.Now() for TimeReceived here? same for ts1 below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no b/c in general TimeRequested happens before TimeReceived. camera.GetImages is the only exception

@viambot 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, 2024
@nicksanford nicksanford merged commit dd5ac3a into viamrobotics:main Dec 13, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants