Inform user if parallelization is disabled at runtime and why #419
Conversation
There was a problem hiding this comment.
@Luc45 Thanks for creating and porting this PR over.
Could you please test this on the command-line ? I'll get the builds running so you can download a PHAR from the CI build artifacts (as you mentioned you don't have a local dev copy of PHPCS).
I suspect you may want to add a PHP_EOL at the end of the messages...
src/Runner.php
Outdated
| if ($this->config->parallel > 1) { | ||
| echo "Parallel processing requires the 'pcntl' PHP extension. Falling back to single-thread execution."; | ||
| } |
There was a problem hiding this comment.
@Luc45 This message would now always display when PHPCS is run on Windows as the pcntl extension is not available on Windows, which makes it very noisy (and not actionable).
I think it may be a better idea to only display this message if (PHP_CODESNIFFER_VERBOSITY > 0). Or maybe it should be silenced completely for Windows ?
There was a problem hiding this comment.
which makes it [...] not actionable
I thought there was a command-line option to set the parallel level, and that the setting is also available in a ruleset.
There was a problem hiding this comment.
Correct, but if the pcntl extension is not available, those options have no effect.
There was a problem hiding this comment.
About Windows, this is a good point.
What about this:
// Check if the PCNTL extension is available.
if (function_exists('pcntl_fork') === false) {
// "pcntl" extension can't be installed on Windows.
if (stripos(PHP_OS, 'WIN') === 0) {
if ($this->config->parallel > 1 && PHP_CODESNIFFER_VERBOSITY > 0) {
echo "Note: Parallel processing is not supported on Windows. Falling back to single-thread execution.";
}
} else {
// For non-Windows environments.
if ($this->config->parallel > 1) {
echo "Parallel processing requires the 'pcntl' PHP extension. Falling back to single-thread execution.";
}
}
$this->config->parallel = 1;
}- If parallel is enabled, it's a UNIX system, and
pcntlis not installed, show a message even without verbosity, to inform the user that his intended behavior is not applying because of a lack of a requirement. - If parallel is enabled, it's Windows, show this message only if verbose mode is enabled, because there's nothing the user can do about it.
- In addition to that, we add a section to "Parallelization" on the Wiki, in the Configuration Options, mentioning the requirement for
pcntland the incompatibility with Windows.
There was a problem hiding this comment.
I'm still not a fan of always displaying this kind of information and I stand by the change I originally suggested.
The things is: parallelization not being available is not blocking in any way. It just means that you are missing out on an optimization, but it will not negatively influence the actual result of the run (and may in actual fact influence it positively).
Accepting this without it being behind the verbosity flag, opens the door to a really noisy display as the next person will say: what about this optional extension ? shouldn't there also be a notice about that ? (pcntl is optional, but there are some other optional extensions in use in PHPCS - with fall-backs).
In the end, we'd have a page long display of warnings before PHPCS even runs. This is not a direction I want to go in or find acceptable to open the door to.
PHPCS 4.0 will already make at least one improvement in that area by displaying certain info to STDERR instead of STDOUT (now everything goes to STDOUT), but we're not at 4.0 yet.
If I'm honest, I think the whole system of runtime errors/warnings/notices related to running PHPCS vs the actual output of a sniff run should be revisited, probably in combination with a different kind of verbosity/debug flag system.
There are already two issues which are related to this open - #30 and #416.
I have some ideas already about this redesign, but there are other things which have much higher priority, so I expect this will have to wait until the 5.0 release.
In the mean time, you have my feedback on what would make the proposed change from this PR acceptable.
- In addition to that, we add a section to "Parallelization" on the Wiki, in the Configuration Options, mentioning the requirement for
pcntland the incompatibility with Windows.
There is already an issue open about this and has been for nearly seven years...
Both threads contain lots of useful information, but someone will need to write this up properly before I can add it to the wiki.
There was a problem hiding this comment.
P.S.: would also be nice to see if some tests could be added to cover this change...
There was a problem hiding this comment.
I misread your initial suggestion as if it were to hide it behind a verbose flag only on Windows, where pcntl is not available.
My rationale to show it without verbose was that we were already inside a $this->config->parallel > 1 conditional, so we would only show it if we were overriding a explicitly-set user-expected behavior. I'm afraid that people rarely runs with --verbose mode enabled, and if they do, an important override like this would hardly be noticed amidst the sea of verbose output.
So truthfully, if hiding it behind a verbose flag, the more impactful "fix" would be just documentation, basically adding a section about Parallelization somewhere and make it clear that the pcntl extension is required. This extension is not installed on PHP by default, so I suspect a large portion of users might actually be missing out on Parallelization, which can have a huge impact on performance.
Having this note behind a verbose flag is better than not having it at all, so I'm fine with making a compromise here and hiding it behind a verbose flag if you think it makes sense?
// Check if the PCNTL extension is available.
if (function_exists('pcntl_fork') === false) {
if ($this->config->parallel > 1 && PHP_CODESNIFFER_VERBOSITY > 0) {
if (stripos(PHP_OS, 'WIN') === 0) {
// "pcntl" extension can't be installed on Windows.
echo "Note: Parallel processing is not supported on Windows. Falling back to single-thread execution.";
} else {
echo "Parallel processing requires the 'pcntl' PHP extension. Falling back to single-thread execution.";
}
}
$this->config->parallel = 1;
}There was a problem hiding this comment.
My rationale to show it without verbose was that we were already inside a
$this->config->parallel > 1conditional, so we would only show it if we were overriding a explicitly-set user-expected behavior.
Except that something like the parallel setting is, in the fast majority of cases, set as a default in a custom ruleset and the actual end-users aren't even aware that the setting is turned on.
Even PHPCS itself has it set up like that.
I'm afraid that people rarely runs with
--verbosemode enabled, and if they do, an important override like this would hardly be noticed amidst the sea of verbose output.
The -v option mostly just displays info about which files are being processed, not much more.
When running with this -v flag - so not the more verbose -vv or -vvv -, the message would be at, or very nearly at, the top of the output where it is hard to miss.
So truthfully, if hiding it behind a verbose flag, the more impactful "fix" would be just documentation, basically adding a section about
Parallelizationsomewhere and make it clear that thepcntlextension is required. This extension is not installed on PHP by default, so I suspect a large portion of users might actually be missing out on Parallelization, which can have a huge impact on performance.
Agreed, see my suggestion about this above as well as all the info in the threads as there is more to it than just that.
I'd definitely welcome a write-up of all the relevant info. A text proposal for the wiki can be added to ticket #10, where this can be discussed further (and which is the more appropriate place for that discussion).
Having this note behind a verbose flag is better than not having it at all, so I'm fine with making a compromise here and hiding it behind a verbose flag if you think it makes sense?
It would. I don't think this is the right implementation though as there are two more places in the same class which change the parallel option (line 109 and line 189), both of which would still not show the warning, even when run with -v.
In future, please open an issue first before submitting a PR as this PR would now need a lot of coaching to get it in a mergable state, which feels like a waste of both of our time.
Seeing the discussion as it is now, I'm leaning towards closing this PR in favour of adding a --diagnose option to PHPCS in v4.x.
I need to gather my thoughts on that a little more, but I think a --diagnose option which will display info about the availability of all optional + required extensions, the PHP version, PHPCS version and some more info would be more useful all-round, as the output of that could then also be used to copy/paste into an issue to ensure all relevant system details are available when analyzing bug reports.
The diagnose option could then also be run "silently" before running PHPCS proper and could display a "Your system does not fulfill all requirements for optimal running, run phpcs --diagnose for full details" message if not all requirements are met.
Once I've gathered my thoughts a little more, I'll open an issue with a proposal for this.
There was a problem hiding this comment.
(line 109 and line 189), both of which would still not show the warning, even when run with -v.
I've seen those before opening the PR, but they are interactive and STDIN mode, respectively, which doesn't make sense to run in Parallel and therefore doesn't need the notice, as there is no initial expectation that they would be parallelized.
I think there's no need to overthink this too much, it's a practical UX issue where a user-defined behavior is silently overridden at runtime. I've opened this PR after spending one hour myself trying to find out if/why Parallel wasn't working, until I opened the PHPCS repository and read the source code of Runner.php (that's how desperate I was) to find out that it was disabled because of the lack of pcntl, which isn't mentioned anywhere, neither. The only way to find out about this is reading the source code directly.
So while we could have --diagnose and a vast documentation about the intricacies of parallel, including how to calculate the core count on every O.S, I think it's paramount to have a very straightforward piece of documentation somewhere, be it at runtime or on the docs, that the user needs pcntl for it to work at all.
What about adding this paragraph to Configuration Options? If you think this notice shouldn't show to the user, maybe we can close this PR and just add this to the docs, then?
Enabling Parallelization
By default, PHP_CodeSniffer will run using a single thread. To take advantage of parallel processing, you can set the default number of threads by using the parallel configuration option.
phpcs --config-set parallel 4Note: Parallel processing requires the 'pcntl' PHP extension. If the extension is not available, PHP_CodeSniffer will default to single-thread execution."
There was a problem hiding this comment.
I agree adding the documentation will be helpful. It will also need a mention of the --parallel=.. CLI argument.
Let's move this discussion to #10 and work on the text there and close this PR.
|
Closing this in favor of documentation in #10 |
|
While it doesn't directly address this issue, PR #447 is a proposal to improve the help screens, including improved information on the help screen about the Reviews and testing of that PR would be appreciated. |
Description
This PR introduces messages to inform users when parallelization is disabled at runtime and the reasons for it. These enhancements aim to improve user awareness in scenarios where parallel processing gets disabled due to unmet conditions like the absence of the
pcntlPHP extension or high verbosity settings. By providing this information, users can better understand the behavior of the system and take appropriate actions if needed.Suggested changelog entry
Related issues/external references
Fixes #
Disclaimer
I'm opening this PR from GitHub web. I don't have a local development copy of PHPCS to test this locally.
Types of changes
PR checklist
package.xmlfile.