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

check_parallel| Adds new test suite for multiple workers/check_parallel #3474

Conversation

doublethefish
Copy link
Contributor

@doublethefish doublethefish commented Apr 4, 2020

Steps

Description

These regression tests put check_parallel() and related functions under test, ensuring that the functionality remains consistent ahead of any bug fixes and performance work we may do.

This makes it easier to make larger changes here later, for example performance work (see PR #3458, and issue #2525).

The test uses the stats generated by a lint-run to regression-test that the correct number of operations have been performed on the expected number of files irrespective of the number of workers being used.

Really this work is a pre-cursor to changing how check_parallel functions (PR #3458). So we check:

  • Fewer workers than files
    • We currently assign individual files to a worker, if we change this it is good to ensure consistent behaviour.
  • Fewer files than workers
    • We want to ensure that no more work than required is done and we saturate as many workers as possible to get the work done quicker (which will happen once we solve the wall-clock issues mentioned in PR #)
  • 1, 2 or 3 checkers
    • We want to ensure that each checker is run just once per file, irrespective of the number of workers or files.
    • Later, if we add more Checker-types that change how we work across workers we will need to ensure that each Checker-type works consistently for all edge cases e,g, see PR Fix #3314 duplicate code error only shows up with pylint jobs 1 #3458 where we add a map/reduce Checker-mixin

Type of Changes

Type
🔨 Refactoring

Related Issue

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 90.534% when pulling 06b1e03 on doublethefish:feature/test_check_parallel into 22fe050 on PyCQA:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 90.534% when pulling 06b1e03 on doublethefish:feature/test_check_parallel into 22fe050 on PyCQA:master.

@coveralls
Copy link

coveralls commented Apr 4, 2020

Coverage Status

Coverage increased (+0.2%) to 90.782% when pulling c85297c on doublethefish:feature/test_check_parallel into 707fc46 on PyCQA:master.

@doublethefish doublethefish changed the title test| -jN| Adds new test suite for multiple workers/check_parallel check_parallel| Adds new test suite for multiple workers/check_parallel Apr 21, 2020
@doublethefish
Copy link
Contributor Author

This is currently broken by #3516. @Pierre-Sassoulas.

@Pierre-Sassoulas
Copy link
Member

Could you try adding missing import in pylint/lint/__init__.py ? As there is no changes in lint.py I think this should be enough.

@doublethefish
Copy link
Contributor Author

Unfortunately that didn't work.

Part of the reason for it is that the new file-module (check_parallel.py) is hidden by the function import: from pylint.lint.check_parallel import check_parallel.

The above means we can't directly access properties in the check_parallel module in a read/write manner and everything else feels like a workaround.

I am going to rename check_parallel.py -> parallel.py, its more descriptive. It looks like that the only file-module with that problem after the refactor.

@doublethefish doublethefish force-pushed the feature/test_check_parallel branch 2 times, most recently from 705d1e7 to 20fd895 Compare May 11, 2020 14:27
@PCManticore PCManticore self-requested a review May 12, 2020 06:52
Copy link
Contributor

@PCManticore PCManticore left a comment

Choose a reason for hiding this comment

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

Hey @doublethefish Thanks for the PR, this has been a great one. I left a bunch of comments that we should address before pulling this in.

tests/test_check_parallel.py Outdated Show resolved Hide resolved
tests/test_check_parallel.py Outdated Show resolved Hide resolved
tests/test_check_parallel.py Outdated Show resolved Hide resolved
tests/test_check_parallel.py Show resolved Hide resolved
tests/test_check_parallel.py Outdated Show resolved Hide resolved
tests/test_check_parallel.py Outdated Show resolved Hide resolved
tests/test_check_parallel.py Outdated Show resolved Hide resolved
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

It seems all of @PCManticore comments were addressed, I think we can merge this. The only thing is that with time a conflict appeared and we can't rebase automatically anymore. Could you rebase on the latest master @doublethefish ?

@Pierre-Sassoulas Pierre-Sassoulas self-assigned this Sep 24, 2020
@brycepg
Copy link
Contributor

brycepg commented Oct 14, 2020

I'd be happy to fix the conflict if you aren't available @doublethefish Will check back later

@brycepg
Copy link
Contributor

brycepg commented Oct 19, 2020

Added this in another PR, thank for the added tests @doublethefish

@brycepg brycepg closed this Oct 19, 2020
@doublethefish
Copy link
Contributor Author

Once again, thanks for sorting this. It's been a difficult 6 weeks.

@doublethefish doublethefish deleted the feature/test_check_parallel branch October 21, 2020 08:03
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 this pull request may close these issues.

5 participants