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.

Sign up for GitHub

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

Jump to bottom

Always do processing in child processes unless --main-process is passed #138

Merged
merged 1 commit into from
Oct 18, 2022
Merged

Always do processing in child processes unless --main-process is passed #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
based on the maximum number of cores detected. Note that the execution within
the main process instead of spawning child processes can be guaranteed by
passing `--main-process`.
- Child processes will always be spawned regardless of the number of items
(known or not), the number of processes defined (or not defined). Instead
the processing will occur only if the `--main-process` option is passed.
- `ContainerAwareCommand` has been removed. `Parallelization` instead provides a
`::getContainer()` method which by defaults returns the Symfony Application
Kernel's container when available.
Expand Down
89 changes: 47 additions & 42 deletions src/Input/ParallelizationInput.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ final class ParallelizationInput
private const MAIN_PROCESS_OPTION = 'main-process';
private const CHILD_OPTION = 'child';

private bool $numberOfProcessesDefined;
private bool $mainProcess;

/**
* @var positive-int
Expand All @@ -50,12 +50,12 @@ final class ParallelizationInput
* @param positive-int|callable():positive-int $numberOfOrFindNumberOfProcesses
*/
public function __construct(
bool $numberOfProcessesDefined,
bool $mainProcess,
$numberOfOrFindNumberOfProcesses,
?string $item,
bool $childProcess
) {
$this->numberOfProcessesDefined = $numberOfProcessesDefined;
$this->mainProcess = $mainProcess;
$this->item = $item;
$this->childProcess = $childProcess;

Expand All @@ -77,45 +77,12 @@ public static function fromInput(InputInterface $input): self
/** @var bool $isChild */
$isChild = $input->getOption(self::CHILD_OPTION);

$numberOfProcessesDefined = null !== $numberOfProcesses;

if ($mainProcess) {
$numberOfProcessesDefined = false;
$validatedNumberOfProcesses = 1;
} elseif ($numberOfProcessesDefined) {
Assert::numeric(
$numberOfProcesses,
sprintf(
'Expected the number of process defined to be a valid numeric value. Got "%s".',
$numberOfProcesses,
),
);

$castedNumberOfProcesses = (int) $numberOfProcesses;

Assert::same(
// We cast it again in string to make sure since it is more convenient to pass an
// int in the tests or when calling the command directly without passing by the CLI
(string) $numberOfProcesses,
(string) $castedNumberOfProcesses,
sprintf(
'Expected the number of process defined to be an integer. Got "%s".',
$numberOfProcesses,
),
);

Assert::greaterThan(
$castedNumberOfProcesses,
0,
sprintf(
'Expected the number of processes to be 1 or greater. Got "%s".',
$castedNumberOfProcesses,
),
);

$validatedNumberOfProcesses = $castedNumberOfProcesses;
} else {
$validatedNumberOfProcesses = static fn () => CpuCoreCounter::getNumberOfCpuCores();
$validatedNumberOfProcesses = null !== $numberOfProcesses
? self::coerceNumberOfProcesses($numberOfProcesses)
: static fn () => CpuCoreCounter::getNumberOfCpuCores();
}

$hasItem = null !== $item;
Expand Down Expand Up @@ -144,7 +111,7 @@ public static function fromInput(InputInterface $input): self
}

return new self(
$numberOfProcessesDefined,
$mainProcess,
$validatedNumberOfProcesses,
$hasItem ? (string) $item : null,
$isChild,
Expand Down Expand Up @@ -184,9 +151,9 @@ public static function configureParallelization(Command $command): void
);
}

public function isNumberOfProcessesDefined(): bool
public function shouldBeProcessedInMainProcess(): bool
{
return $this->numberOfProcessesDefined;
return $this->mainProcess;
}

/**
Expand All @@ -210,4 +177,42 @@ public function isChildProcess(): bool
{
return $this->childProcess;
}

/**
* @return positive-int
*/
private static function coerceNumberOfProcesses(string $numberOfProcesses): int
{
Assert::numeric(
$numberOfProcesses,
sprintf(
'Expected the number of process defined to be a valid numeric value. Got "%s".',
$numberOfProcesses,
),
);

$castedNumberOfProcesses = (int) $numberOfProcesses;

Assert::same(
// We cast it again in string to make sure since it is more convenient to pass an
// int in the tests or when calling the command directly without passing by the CLI
(string) $numberOfProcesses,
(string) $castedNumberOfProcesses,
sprintf(
'Expected the number of process defined to be an integer. Got "%s".',
$numberOfProcesses,
),
);

Assert::greaterThan(
$castedNumberOfProcesses,
0,
sprintf(
'Expected the number of processes to be 1 or greater. Got "%s".',
$castedNumberOfProcesses,
),
);

return $castedNumberOfProcesses;
}
}
22 changes: 1 addition & 21 deletions src/ParallelExecutor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,7 @@ private function executeMainProcess(

$numberOfItems = $itemIterator->getNumberOfItems();

$shouldSpawnChildProcesses = self::shouldSpawnChildProcesses(
$numberOfItems,
$desiredSegmentSize,
$numberOfProcesses,
$parallelizationInput->isNumberOfProcessesDefined(),
);
$shouldSpawnChildProcesses = !$parallelizationInput->shouldBeProcessedInMainProcess();

$configuration = Configuration::create(
$shouldSpawnChildProcesses,
Expand Down Expand Up @@ -340,21 +335,6 @@ private function runTolerantSingleCommand(
}
}

/**
* @param 0|positive-int|null $numberOfItems
* @param positive-int $segmentSize
* @param positive-int $numberOfProcesses
*/
private static function shouldSpawnChildProcesses(
?int $numberOfItems,
int $segmentSize,
int $numberOfProcesses,
bool $numberOfProcessesDefined
): bool {
return (null === $numberOfItems || $numberOfItems > $segmentSize)
&& ($numberOfProcesses > 1 || $numberOfProcessesDefined);
}

/**
* @param int<1,max> $segmentSize
* @param int<1,max> $numberOfProcesses
Expand Down
34 changes: 27 additions & 7 deletions tests/Input/ParallelizationInputTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public function test_it_can_be_instantiated(): void
true,
);

self::assertTrue($input->isNumberOfProcessesDefined());
self::assertTrue($input->shouldBeProcessedInMainProcess());
self::assertSame(5, $input->getNumberOfProcesses());
self::assertSame('item', $input->getItem());
self::assertTrue($input->isChildProcess());
Expand All @@ -81,7 +81,7 @@ static function () use (&$closureEvaluated): int {
true,
);

self::assertTrue($input->isNumberOfProcessesDefined());
self::assertTrue($input->shouldBeProcessedInMainProcess());
self::assertSame('item', $input->getItem());
self::assertTrue($input->isChildProcess());

Expand Down Expand Up @@ -131,10 +131,10 @@ public function test_it_can_be_instantiated_from_an_input_with_an_item_for_a_chi
ParallelizationInput::fromInput($input);
}

public function test_it_can_be_instantiated_from_an_input_with_an_invalid_number_of_processes(): void
public function test_it_cannot_be_instantiated_from_an_input_with_an_invalid_number_of_processes(): void
{
$input = new ArrayInput([
'--processes' => 0,
'--processes' => '0',
]);
self::bindInput($input);

Expand Down Expand Up @@ -176,7 +176,7 @@ public static function inputProvider(): iterable
yield 'number of process defined: 1' => [
new StringInput('--processes=1'),
new ParallelizationInput(
true,
false,
1,
null,
false,
Expand All @@ -186,7 +186,7 @@ public static function inputProvider(): iterable
yield 'number of process defined: 4' => [
new StringInput('--processes=4'),
new ParallelizationInput(
true,
false,
4,
null,
false,
Expand Down Expand Up @@ -233,10 +233,30 @@ public static function inputProvider(): iterable
),
];

yield 'do processing in the main process' => [
new StringInput('--main-process --processes 15'),
new ParallelizationInput(
true,
1,
null,
false,
),
];

yield 'do processing in the main process without number of processes specified' => [
new StringInput('--main-process'),
new ParallelizationInput(
true,
1,
null,
false,
),
];

yield 'nominal' => [
new StringInput('--child --processes 15'),
new ParallelizationInput(
true,
false,
15,
null,
true,
Expand Down
6 changes: 3 additions & 3 deletions tests/Integration/ParallelizationIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public function test_it_can_run_the_command_with_multiple_processes(): void
$commandTester->execute(
[
'command' => 'import:movies',
'--processes' => 2,
'--processes' => '2',
],
['interactive' => true],
);
Expand Down Expand Up @@ -143,7 +143,7 @@ public function test_it_can_run_the_command_with_multiple_processes_without_know
$commandTester->execute(
[
'command' => 'import:movies-unknown-count',
'--processes' => 2,
'--processes' => '2',
],
['interactive' => true],
);
Expand Down Expand Up @@ -172,7 +172,7 @@ public function test_it_can_run_the_command_with_multiple_processes_in_debug_mod
$commandTester->execute(
[
'command' => 'import:movies',
'--processes' => 2,
'--processes' => '2',
],
[
'interactive' => true,
Expand Down
Loading