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.

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

How to make --parallel and --cache false work together? #9744

Closed
Aqualon opened this issue Apr 28, 2021 · 3 comments · Fixed by #9795
Closed

How to make --parallel and --cache false work together? #9744

Aqualon opened this issue Apr 28, 2021 · 3 comments · Fixed by #9795

Comments

@Aqualon
Copy link
Contributor

Aqualon commented Apr 28, 2021

I'm using the linter-rubocop package in Atom, that sets --cache false for calls to rubocop. Since activation of --parallel via .rubocop file in my project, I get the error -P/--parallel uses caching to speed up execution, so combining with --cache false is not allowed.


Expected behavior

I see two options:

  • Deactivate --parallel if --cache false is set, similar to
    def disable_parallel_when_invalid_combo
    combos = {
    auto_gen_config: '--auto-gen-config',
    fail_fast: '-F/--fail-fast.',
    auto_correct: '--auto-correct.'
    }
  • Allow --no-parallel CLI option to override the --parallel from .rubocop file.

Eventually bundle exec rubocop --cache false [--no-parallel] should run rubocop and not output
-P/--parallel uses caching to speed up execution, so combining with --cache false is not allowed.

Actual behavior

Rubocop prints -P/--parallel uses caching to speed up execution, so combining with --cache false is not allowed.

Steps to reproduce the problem

Add --parallel to .rubocop file and run bundle exec rubocop --cache false [--no-parallel]

RuboCop version

$ [bundle exec] rubocop -V
1.13.0 (using Parser 3.0.1.0, rubocop-ast 1.4.1, running on ruby 3.0.1 x86_64-darwin20)
  - rubocop-performance 1.10.2
  - rubocop-rails 2.9.1
  - rubocop-rspec 2.2.0
@rrosenblum
Copy link
Contributor

Unfortunately this is going to take a large rewrite of how some of the internals of RuboCop work. The current strategy for parallel relies on caching because it splits the files that will be scanned by RuboCop into different groups, invokes RuboCop X number times on the subset groups of files with caching enabled, then once each "sub job" is finished, it reruns RuboCop over the entire project. This last run picks up the cache from the parallel run allowing it run quickly. The whole process is still faster than running RuboCop without cache or parallel.

It's been a while since I looked into this. I think there's been some work to help improve the situation so that we might be able to use a real parallel at some point. I think there have been challenges around the internal design and being able to split work based on cop or file.

I'm curious why linter-rubocop would force disable caching

@Aqualon
Copy link
Contributor Author

Aqualon commented Apr 30, 2021

I found AtomLinter/linter-rubocop@7579065, maybe @vzamanillo can share more details?

I'd also be fine with having parallel automatically deactivated, don't know why in

if @options[:cache] == 'false'
raise OptionArgumentError, '-P/--parallel uses caching to speed up ' \
'execution, so combining with --cache ' \
'false is not allowed.'
end
disable_parallel_when_invalid_combo
it's distinguished between the other cases where parallel is ignored and the cache = false case, where it's raising an exception.

@rrosenblum
Copy link
Contributor

I wasn't even thinking about disabling parallel when cache is disabled until later last night. disable_parallel_when_invalid_combo is a very new feature that was added a couple of weeks ago. The cache: false option was being handled slightly differently than the other config options which is why I probably overlooked it. Most likely it's because this is checking for a "disabled" config instead of an "enabled" config. I think it would be a good idea to update the code to auto disable parallel when cache is set to false. As with the other conflicting config options, we would still print a warning message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants