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

Inform user if parallelization is disabled at runtime and why #419

Closed
wants to merge 2 commits into from
Closed

Inform user if parallelization is disabled at runtime and why #419

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
100cebd
Inform user if parallelization is disabled at runtime and why
Luc45 Mar 28, 2024
7841799
Update Runner.php
Luc45 Apr 1, 2024
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
6 changes: 6 additions & 0 deletions src/Runner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,11 +425,17 @@ private function run()
// If verbosity is too high, turn off parallelism so the
// debug output is clean.
if (PHP_CODESNIFFER_VERBOSITY > 1) {
if ($this->config->parallel > 1) {
echo "Parallel processing disabled for clearer output at higher verbosity levels.";
}
$this->config->parallel = 1;
}

// If the PCNTL extension isn't installed, we can't fork.
if (function_exists('pcntl_fork') === false) {
if ($this->config->parallel > 1) {
echo "Parallel processing requires the 'pcntl' PHP extension. Falling back to single-thread execution.";
}
Copy link
Member

Choose a reason for hiding this comment

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

@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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, but if the pcntl extension is not available, those options have no effect.

Copy link
Author

@Luc45 Luc45 Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 pcntl is 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 pcntl and the incompatibility with Windows.

Copy link
Member

Choose a reason for hiding this comment

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

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 pcntl and 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.

Copy link
Member

Choose a reason for hiding this comment

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

P.S.: would also be nice to see if some tests could be added to cover this change...

Copy link
Author

Choose a reason for hiding this comment

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

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;
}

Copy link
Member

@jrfnl jrfnl Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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 --verbose mode 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 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.

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.

Copy link
Author

@Luc45 Luc45 Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(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 4

Note: Parallel processing requires the 'pcntl' PHP extension. If the extension is not available, PHP_CodeSniffer will default to single-thread execution."

Copy link
Member

Choose a reason for hiding this comment

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

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.

$this->config->parallel = 1;
}

Expand Down
Loading