From 423842e7dcf49285b91d113038346184670aa015 Mon Sep 17 00:00:00 2001 From: Martin Zurowietz Date: Tue, 15 Feb 2022 16:40:39 +0100 Subject: [PATCH 1/2] Use model factories of Laravel 8 --- tests/VolumeTest.php | 49 ++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/tests/VolumeTest.php b/tests/VolumeTest.php index c96bc082..f373ee1b 100644 --- a/tests/VolumeTest.php +++ b/tests/VolumeTest.php @@ -25,12 +25,15 @@ public function testReadyForDelphiDetection() $volume = Volume::convert(BaseVolumeTest::create()); $label = LabelTest::create(); - $images = factory(Image::class, 4)->create() - ->each(function ($i) use ($volume) { - $i->filename = uniqid(); - $i->volume_id = $volume->id; - $i->save(); - }); + $images = Image::class::factory() + ->count(4) + ->sequence(function ($i) use ($volume) { + return [ + 'filename' => uniqid(), + 'volume_id' => $volume->id, + ]; + }) + ->create(); try { $volume->readyForDelphiDetection($label); @@ -84,14 +87,16 @@ public function testReadyForDelphiDetectionOutOfBounds() $volume = Volume::convert(BaseVolumeTest::create()); $label = LabelTest::create(); - $images = factory(Image::class, 4)->create() - ->each(function ($i) use ($volume) { - $i->filename = uniqid(); - $i->volume_id = $volume->id; - $i->width = 100; - $i->height = 100; - $i->save(); - }); + $images = Image::class::factory() + ->count(4) + ->sequence(function ($i) use ($volume) { + return [ + 'filename' => uniqid(), + 'volume_id' => $volume->id, + 'attrs' => ['width' => 100, 'height' => 100], + ]; + }) + ->create(); $images->each(function ($i) use ($label) { ImageTest::addLaserpoints($i, $label, 2); @@ -115,12 +120,16 @@ public function testReadyForDelphiDetectionOutOfBounds() public function testHasDetectedLaserpoints() { $volume = Volume::convert(BaseVolumeTest::create()); - $images = factory(Image::class, 4)->create() - ->each(function ($i) use ($volume) { - $i->filename = uniqid(); - $i->volume_id = $volume->id; - $i->save(); - }); + + $images = Image::class::factory() + ->count(4) + ->sequence(function ($i) use ($volume) { + return [ + 'filename' => uniqid(), + 'volume_id' => $volume->id, + ]; + }) + ->create(); $this->assertFalse($volume->hasDetectedLaserpoints()); From 5f395e0aae10c87b1158ddaa05ccb64b6b5202cc Mon Sep 17 00:00:00 2001 From: Martin Zurowietz Date: Fri, 18 Feb 2022 16:15:17 +0100 Subject: [PATCH 2/2] Use job batching to process laser point detection --- src/Jobs/ProcessDelphiJob.php | 50 ++------------------ src/Jobs/ProcessImageDelphiJob.php | 12 ++++- src/Jobs/ProcessVolumeDelphiJob.php | 29 +++++++----- tests/Jobs/ProcessDelphiJobTest.php | 56 ----------------------- tests/Jobs/ProcessImageDelphiJobTest.php | 13 ++++-- tests/Jobs/ProcessVolumeDelphiJobTest.php | 39 +++++----------- 6 files changed, 50 insertions(+), 149 deletions(-) diff --git a/src/Jobs/ProcessDelphiJob.php b/src/Jobs/ProcessDelphiJob.php index daceedcc..a178870e 100644 --- a/src/Jobs/ProcessDelphiJob.php +++ b/src/Jobs/ProcessDelphiJob.php @@ -10,6 +10,7 @@ use Exception; use File; use FileCache; +use Illuminate\Bus\Batchable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; @@ -17,7 +18,7 @@ class ProcessDelphiJob extends BaseJob implements ShouldQueue { - use InteractsWithQueue, SerializesModels; + use Batchable, InteractsWithQueue, SerializesModels; /** * The image to process. @@ -40,14 +41,6 @@ class ProcessDelphiJob extends BaseJob implements ShouldQueue */ protected $distance; - /** - * Key of the cached count of other jobs that run on the same volume than this one. - * The last job should delete the gatherFile. - * - * @var string - */ - protected $cacheKey; - /** * Ignore this job if the image does not exist any more. * @@ -72,13 +65,11 @@ class ProcessDelphiJob extends BaseJob implements ShouldQueue * * @return void */ - public function __construct($image, $distance, $gatherFile, $cacheKey = null) + public function __construct($image, $distance, $gatherFile) { $this->image = Image::convert($image); $this->gatherFile = $gatherFile; $this->distance = $distance; - // If null, this job assumes it is the only one accessing the gatherFile. - $this->cacheKey = $cacheKey; } /** @@ -115,40 +106,5 @@ public function handle() $this->image->laserpoints = $output; $this->image->save(); - - $this->maybeDeleteGatherFile(); - } - - /** - * Handle a job failure. - * - * @return void - */ - public function failed() - { - $this->maybeDeleteGatherFile(); - } - - /** - * Handles the deletion of the gatherFile once all "sibling" jobs finished. - * - * If more than one image is processed during a Delphi LP detection, the jobs use the - * cache to track how many of them are still running. They need to track this to - * determine when the gatherFile can be deleted. This function updates the count - * when a job was finished and deletes the gather file if this is the last job to - * finish. - */ - protected function maybeDeleteGatherFile() - { - if ($this->cacheKey) { - // This requires an atomic operation to work correctly. BIIGLE uses the - // Redis cache which provides these atomic operations. - if (Cache::decrement($this->cacheKey) > 0) { - return; - } - Cache::forget($this->cacheKey); - } - - Storage::disk(config('laserpoints.disk'))->delete($this->gatherFile); } } diff --git a/src/Jobs/ProcessImageDelphiJob.php b/src/Jobs/ProcessImageDelphiJob.php index 41d4e7bc..17139e98 100644 --- a/src/Jobs/ProcessImageDelphiJob.php +++ b/src/Jobs/ProcessImageDelphiJob.php @@ -3,6 +3,8 @@ namespace Biigle\Modules\Laserpoints\Jobs; use Biigle\Image; +use Illuminate\Support\Facades\Bus; +use Storage; class ProcessImageDelphiJob extends Job { @@ -42,7 +44,13 @@ public function handle() $points = $this->getLaserpointsForVolume($this->image->volume_id); $gatherFile = $this->gather($points); - ProcessDelphiJob::dispatch($this->image, $this->distance, $gatherFile) - ->onQueue(config('laserpoints.process_delphi_queue')); + $job = new ProcessDelphiJob($this->image, $this->distance, $gatherFile); + + Bus::batch([$job]) + ->onQueue(config('laserpoints.process_delphi_queue')) + ->finally(function () use ($gatherFile) { + Storage::disk(config('laserpoints.disk'))->delete($gatherFile); + }) + ->dispatch(); } } diff --git a/src/Jobs/ProcessVolumeDelphiJob.php b/src/Jobs/ProcessVolumeDelphiJob.php index 903ec2e6..12c05b50 100644 --- a/src/Jobs/ProcessVolumeDelphiJob.php +++ b/src/Jobs/ProcessVolumeDelphiJob.php @@ -6,6 +6,8 @@ use Biigle\Volume; use Cache; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\Bus; +use Storage; class ProcessVolumeDelphiJob extends Job { @@ -50,25 +52,30 @@ public function handle() $points = $this->getLaserpointsForVolume($this->volume->id); $images = Image::whereIn('id', $points->keys())->get(); - foreach ($images as $image) { - ProcessManualJob::dispatch($image, $points->get($image->id), $this->distance) - ->onQueue(config('laserpoints.process_manual_queue')); - } + $jobs = $images->map(function ($image) use ($points) { + return new ProcessManualJob($image, $points->get($image->id), $this->distance); + }); + Bus::batch($jobs) + ->onQueue(config('laserpoints.process_manual_queue')) + ->dispatch(); $query = $this->volume->images()->whereNotIn('id', $points->keys()); if ($query->exists()) { - $imagesToProcess = $query->get(); + $images = $query->get(); $gatherFile = $this->gather($points); - $cacheKey = uniqid('delphi_job_count_'); - Cache::forever($cacheKey, $imagesToProcess->count()); + $jobs = $images->map(function ($image) use ($gatherFile) { + return new ProcessDelphiJob($image, $this->distance, $gatherFile); + }); - foreach ($imagesToProcess as $image) { - ProcessDelphiJob::dispatch($image, $this->distance, $gatherFile, $cacheKey) - ->onQueue(config('laserpoints.process_delphi_queue')); - } + Bus::batch($jobs) + ->onQueue(config('laserpoints.process_delphi_queue')) + ->finally(function () use ($gatherFile) { + Storage::disk(config('laserpoints.disk'))->delete($gatherFile); + }) + ->dispatch(); } } } diff --git a/tests/Jobs/ProcessDelphiJobTest.php b/tests/Jobs/ProcessDelphiJobTest.php index 97d3ad1a..79605136 100644 --- a/tests/Jobs/ProcessDelphiJobTest.php +++ b/tests/Jobs/ProcessDelphiJobTest.php @@ -64,54 +64,10 @@ public function testHandle() $this->assertEquals($expect, $this->image->fresh()->laserpoints); // Previously set attrs should not be lost. $this->assertEquals(1, $this->image->fresh()->attrs['a']); - $this->assertFalse($disk->exists($this->gatherFile)); - } - - public function testHandleCountDecrease() - { - $disk = Storage::disk('laserpoints'); - $disk->put($this->gatherFile, 'test'); - Cache::forever('test_job_count', 2); - - $mock = Mockery::mock(DelphiApply::class); - $mock->shouldReceive('execute') - ->once() - ->andReturn([]); - - App::singleton(DelphiApply::class, function () use ($mock) { - return $mock; - }); - - with(new ProcessDelphiJob($this->image, 30, $this->gatherFile, 'test_job_count'))->handle(); - - $this->assertEquals(1, Cache::get('test_job_count')); - $this->assertTrue($disk->exists($this->gatherFile)); - } - - public function testHandleCountZero() - { - $disk = Storage::disk('laserpoints'); - $disk->put($this->gatherFile, 'test'); - Cache::forever('test_job_count', 1); - - $mock = Mockery::mock(DelphiApply::class); - $mock->shouldReceive('execute') - ->once() - ->andReturn([]); - - App::singleton(DelphiApply::class, function () use ($mock) { - return $mock; - }); - - with(new ProcessDelphiJob($this->image, 30, $this->gatherFile, 'test_job_count'))->handle(); - $this->assertFalse(Cache::has('test_job_count')); - $this->assertFalse($disk->exists($this->gatherFile)); } public function testHandleGracefulError() { - $disk = Storage::disk('laserpoints'); - $disk->put($this->gatherFile, 'test'); $mock = Mockery::mock(DelphiApply::class); $mock->shouldReceive('execute') ->once() @@ -133,13 +89,10 @@ public function testHandleGracefulError() ]; $this->assertEquals($expect, $this->image->fresh()->laserpoints); - $this->assertFalse($disk->exists($this->gatherFile)); } public function testHandleFatalError() { - $disk = Storage::disk('laserpoints'); - $disk->put($this->gatherFile, 'test'); // previous laserpoint detection results should be removed $this->image->laserpoints = [ 'area' => 100, @@ -169,14 +122,5 @@ public function testHandleFatalError() ]; $this->assertEquals($expect, $this->image->fresh()->laserpoints); - $this->assertFalse($disk->exists($this->gatherFile)); - } - - public function testFailed() - { - $disk = Storage::disk('laserpoints'); - $disk->put($this->gatherFile, 'test'); - with(new ProcessDelphiJob($this->image, 30, $this->gatherFile))->failed(); - $this->assertFalse($disk->exists($this->gatherFile)); } } diff --git a/tests/Jobs/ProcessImageDelphiJobTest.php b/tests/Jobs/ProcessImageDelphiJobTest.php index 9a6c2dcd..731b55c2 100644 --- a/tests/Jobs/ProcessImageDelphiJobTest.php +++ b/tests/Jobs/ProcessImageDelphiJobTest.php @@ -4,16 +4,15 @@ use Biigle\Modules\Laserpoints\Jobs\ProcessDelphiJob; use Biigle\Modules\Laserpoints\Jobs\ProcessImageDelphiJob; -use Biigle\Modules\Laserpoints\Support\DelphiGather; use Biigle\Shape; use Biigle\Tests\ImageAnnotationLabelTest; use Biigle\Tests\ImageAnnotationTest; use Biigle\Tests\ImageTest; use Biigle\Tests\LabelTest; -use Mockery; -use Queue; use Storage; use TestCase; +use Illuminate\Bus\PendingBatch; +use Illuminate\Support\Facades\Bus; class ProcessImageDelphiJobTest extends TestCase { @@ -39,10 +38,14 @@ public function testHandle() 'volume_id' => $image->volume_id, ]); - Queue::fake(); + Bus::fake(); $job = new ProcessImageDelphiJobStub($image2, 50, $label->id); $job->handle(); - Queue::assertPushed(ProcessDelphiJob::class); + + Bus::assertBatched(function (PendingBatch $batch) { + return $batch->jobs->count() === 1 && $batch->jobs[0] instanceof ProcessDelphiJob; + }); + $points = $job->points->toArray(); $this->assertArrayHasKey($image->id, $points); $points = $points[$image->id]; diff --git a/tests/Jobs/ProcessVolumeDelphiJobTest.php b/tests/Jobs/ProcessVolumeDelphiJobTest.php index bf4b4ee3..c14adba7 100644 --- a/tests/Jobs/ProcessVolumeDelphiJobTest.php +++ b/tests/Jobs/ProcessVolumeDelphiJobTest.php @@ -11,12 +11,11 @@ use Biigle\Tests\LabelTest; use Biigle\Tests\Modules\Laserpoints\ImageTest as LpImageTest; use Biigle\Tests\VolumeTest; -use Cache; use FileCache; -use Mockery; -use Queue; use Storage; use TestCase; +use Illuminate\Bus\PendingBatch; +use Illuminate\Support\Facades\Bus; class ProcessVolumeDelphiJobTest extends TestCase { @@ -37,38 +36,22 @@ public function testHandle() 'volume_id' => $image->volume_id, ]); - Cache::shouldReceive('rememberForever')->andReturn(Shape::whereName('Point')->first()); - Cache::shouldReceive('forever')->once()->with(Mockery::any(), 1); + Bus::fake(); - Queue::fake(); $job = new ProcessVolumeDelphiJobStub($image->volume, 50, $label->id); $job->handle(); - Queue::assertPushed(ProcessDelphiJob::class, 1); - Queue::assertPushed(ProcessManualJob::class); + + $expect = [$image->id => '[[0,0],[0,0],[0,0]]']; $this->assertEquals($expect, $job->points->toArray()); - } - - public function testCacheKey() - { - $label = LabelTest::create(); - $volume = VolumeTest::create(); - for ($i = 0; $i < 2; $i++) { - $this->createAnnotatedImage($label, $volume->id); - ImageTest::create([ - 'volume_id' => $volume->id, - 'filename' => uniqid(), - ]); - } - Cache::shouldReceive('rememberForever')->andReturn(Shape::whereName('Point')->first()); - Cache::shouldReceive('forever')->once()->with(Mockery::any(), 2); + Bus::assertBatched(function (PendingBatch $batch) { + return $batch->jobs->count() === 1 && $batch->jobs[0] instanceof ProcessDelphiJob; + }); - Queue::fake(); - $job = new ProcessVolumeDelphiJobStub($volume, 50, $label->id); - $job->handle(); - Queue::assertPushed(ProcessDelphiJob::class, 2); - Queue::assertPushed(ProcessManualJob::class); + Bus::assertBatched(function (PendingBatch $batch) { + return $batch->jobs->count() === 1 && $batch->jobs[0] instanceof ProcessManualJob; + }); } protected function createAnnotatedImage($label, $volumeId = null)