[stability] Survive vips sigabrt #75

blefaudeux · 2025-01-24T19:59:55Z

For some payloads vips will sigabrt on a function call, without an obvious way to catch this beforehand (initial loadbuffer doesn't error out, image characteristics have nothing specific). Happening very often on a given cluster, guess is that network could be less reliable and broken packets would not be caught somehow.

Adding a relatively ugly but seemingly reliable defer(recover) pattern which will skip this payload for now, testing overnight. Not sure how to unit test this

Note that this PR enables img.Close()-ing the vips image when done, which lowers the RAM use quite dramatically (not waiting for the go GC)

blefaudeux · 2025-01-24T20:03:04Z

cmd/db/main.go

@@ -0,0 +1,90 @@
+package main


a binary to stress test the lib in the vectorDB path

blefaudeux · 2025-01-24T20:03:35Z

cmd/filesystem/main.go

@@ -29,11 +29,9 @@ func main() {
 	sourceConfig.Rank = 0
 	sourceConfig.WorldSize = 1



a binary to stress test the lib in the filesystem path

blefaudeux · 2025-01-24T20:03:59Z

pkg/client.go

@@ -161,12 +167,8 @@ type DatagoClient struct {

 // GetClient is a constructor for the DatagoClient, given a JSON configuration string
 func GetClient(config DatagoConfig) *DatagoClient {
-	// Make sure that the GC is run more often than usual
-	// VIPS will allocate a lot of memory and we want to make sure that it's released as soon as possible
-	os.Setenv("GOGC", "10") // Default is 100, we're running it when heap is 10% larger than the last GC


this was ugly, not required when we properly release the buffers (and it doesn't crash on us)

blefaudeux · 2025-01-24T20:04:41Z

pkg/client.go

 	// Initialize the vips library
-	err := os.Setenv("VIPS_DISC_THRESHOLD", "5g")
+	err := os.Setenv("VIPS_DISC_THRESHOLD", "10g")


in other terms: never go do disk. Could be changed depending on where this runs, 2TB servers for Photoroom so ram is fine

blefaudeux · 2025-01-24T20:05:19Z

pkg/serdes.go

 	}

-	err = img.AutoRotate()


not required anymore, can be done at load time, a little cleaner

blefaudeux · 2025-01-24T20:05:37Z

pkg/serdes.go

-	// Decode the image payload using vips
-	img, err := vips.NewImageFromBuffer(buffer)
+	// Decode the image payload using vips, using bulletproof settings
+	importParams := vips.NewImportParams()


explicit and visible load settings, didn't make anything more stable but still a good thing probably

blefaudeux · 2025-01-24T20:06:00Z

pkg/serdes.go

 	// If the image is 4 channels, we need to drop the alpha channel
 	if img.Bands() == 4 {
 		err = img.Flatten(&vips.Color{R: 255, G: 255, B: 255}) // Flatten with white background
 		if err != nil {
-			fmt.Println("Error flattening image:", err)


code style, reporting these messages directly in the error, nicer

blefaudeux · 2025-01-24T20:06:32Z

pkg/serdes.go

+				return nil, -1., fmt.Errorf("error converting to sRGB: %w", err)
+			}
+		} else {
+			// // FIXME: maybe that we could recover these still. By default throws an error, sRGB and PNG not supported


cleaner conversion, vips would complain that we were doing something illegal for these images so skipping it now

blefaudeux · 2025-01-24T20:07:58Z

pkg/serdes.go

+	// Optionally crop and resize the image on the fly. Save the aspect ratio in the process for future use
+	originalWidth, originalHeight := img.Width(), img.Height()
+
+	if transform != nil {


the crux of the stability fix when direct-to-cluster, the subsequent vips call can trigger a sigabrt and I couldn't find any way to catch this case beforehand.
Flipping this around, this can happen but we recover from it. Guess is that some network transmission was broken and we didn't catch it as we should have

blefaudeux · 2025-01-24T20:08:34Z

pkg/serdes.go

-			if err != nil {
-				return nil, -1., err
-			}
+			imgBytes, _, err = img.ExportPng(&vips.PngExportParams{


same as in the loading case, be explicit above the params used here instead of using ones which would depend on the library defaults

blefaudeux · 2025-01-24T20:09:16Z

pkg/serdes.go

 		// Define bit depth de facto, not exposed in the vips interface
 		bitDepth = len(imgBytes) / (width * height * channels) * 8 // 8 bits per byte
 	}

+	defer img.Close() // release vips buffers when done


this helps a lot in high throuhgput situations, lowers the RAM use and just generally makes sense, we're done with this vips intermediate at this point

blefaudeux · 2025-01-24T20:10:00Z

python/benchmark_db.py

-    client_config.SourceConfig = source_config
-    client = datago.GetClient(client_config)
-
+    client_config = {


previous implementation was buggy, in that the update to the config object in the python space wouldn't be reflected in go. This json/dict approach is readable and not broken

blefaudeux · 2025-01-25T07:31:42Z

can still crash eventually

- now reproed the issue, looks like it's around the autorotate part - fixing this hopefully

blefaudeux · 2025-01-26T13:32:13Z

can still be made to hard crash, when trying long enough.

blefaudeux · 2025-01-28T17:58:16Z

reviving this for testing

blefaudeux commented Jan 24, 2025

View reviewed changes

cmd/db/main.go

@@ -0,0 +1,90 @@

package main

Copy link

Contributor Author

blefaudeux Jan 24, 2025

Choose a reason for hiding this comment

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

a binary to stress test the lib in the vectorDB path

blefaudeux commented Jan 24, 2025

View reviewed changes

pkg/serdes.go

}

err = img.AutoRotate()

Copy link

Contributor Author

blefaudeux Jan 24, 2025

Choose a reason for hiding this comment

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

not required anymore, can be done at load time, a little cleaner

blefaudeux commented Jan 24, 2025

View reviewed changes

blefaudeux force-pushed the ben/img_close branch 3 times, most recently from 012c695 to e8b826a Compare January 24, 2025 22:50

blefaudeux marked this pull request as draft January 25, 2025 07:31

blefaudeux force-pushed the ben/img_close branch 2 times, most recently from be2d5e6 to 678303c Compare January 25, 2025 15:32

- bench tools for both paths

e957b2e

- now reproed the issue, looks like it's around the autorotate part - fixing this hopefully

blefaudeux force-pushed the ben/img_close branch from 678303c to e957b2e Compare January 25, 2025 15:33

blefaudeux closed this Jan 26, 2025

blefaudeux reopened this Jan 28, 2025

Benjamin Lefaudeux added 2 commits January 28, 2025 19:09

Merge branch 'main' into ben/img_close

09a60cf

linting

bf6992a

blefaudeux marked this pull request as ready for review January 28, 2025 19:34

adding a basic test

d04f7ff

blefaudeux force-pushed the ben/img_close branch from 494df94 to d04f7ff Compare January 28, 2025 19:36

blefaudeux merged commit a46e601 into main Jan 28, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stability] Survive vips sigabrt #75

[stability] Survive vips sigabrt #75

blefaudeux commented Jan 24, 2025 •

edited

Loading

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux Jan 24, 2025

blefaudeux commented Jan 25, 2025

blefaudeux commented Jan 26, 2025

blefaudeux commented Jan 28, 2025

		@@ -29,11 +29,9 @@ func main() {
		sourceConfig.Rank = 0
		sourceConfig.WorldSize = 1

[stability] Survive vips sigabrt #75

[stability] Survive vips sigabrt #75

Conversation

blefaudeux commented Jan 24, 2025 • 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

blefaudeux commented Jan 25, 2025

blefaudeux commented Jan 26, 2025

blefaudeux commented Jan 28, 2025

blefaudeux commented Jan 24, 2025 •

edited

Loading