Fixes #8625: Split linters to run for a specific file extension #8656

Hudda · 2020-02-16T17:21:40Z

Explanation

Fixes Splitting lint checks into Python, Js, Ts lint checks. #8625
Split the linter in pre_commit_linter to run to run linter on a specific file extension type
Example: python -m scripts.pre_commit_linter --only-check-file-extensions py to run linter for files with '.py' extensions.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python -m scripts.pre_commit_linter and python -m scripts.run_frontend_tests.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "PR CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

oppiabot · 2020-02-16T17:21:41Z

Hi, @Hudda. This pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

oppiabot · 2020-02-16T17:29:11Z

Assigning @kevinlee12 for the first-pass review of this pull request. Thanks!

kevinlee12

thanks for the PR! Even though this might alleviate the issue, it's better to use the semaphore method, see https://github.com/oppia/oppia/blob/develop/scripts/run_backend_tests.py#L255 (and generally how the backend tests are structured). Semaphores essentially use an OS level way to doing resource control that is most efficient to the OS. The work here will still be needed since each linting operation needs to be divided into distinct chunks. Also, it's worth keeping in mind that pylint and eslint most likely has some parallelization underneath the hood, so it's best to have those lint by themselves. let me know if you have any questions, I'm happy to scope them out in more detail if you'd like.

seanlip · 2020-02-17T05:46:23Z

Just wanted to say that I agree with following the way we approach parallelization in run_backend_tests.py. One thing you might want to consider is to split the common "task running" logic that's currently in run_backend_tests.py out into a separate module. Then, both run_backend_tests.py and pre_commit_linter.py can use the code in that module to manage tasks, and you won't need to reimplement that logic from scratch in pre_commit_linter.py.

oppiabot · 2020-02-24T06:28:20Z

Hi @Hudda, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

Hudda · 2020-02-29T16:20:17Z

@kevinlee12 PTAL! Also, pylint is running twice, I was not be able to find a reason for that. Can you help me with it?

kevinlee12

thanks @Hudda, this is moving in the right direction, but it needs more isolation built in. see my comments for details. thanks!

kevinlee12 · 2020-02-29T18:32:11Z

scripts/pre_commit_linter.py

 def _join_linting_process(linting_processes, result_queues, result_stdouts):
-    """Join process spawn off by _lint_all_files and capture the outputs."""
+    """Join process spawn off by lint_all_files and capture the outputs."""
    for process in linting_processes:
        process.join()


This is not being used any more, remove.

kevinlee12 · 2020-02-29T20:19:35Z

scripts/semaphore_method.py

@@ -0,0 +1,128 @@
+# coding: utf-8
+#
+# Copyright 2018 The Oppia Authors. All Rights Reserved.


kevinlee12 · 2020-02-29T20:21:47Z

scripts/pre_commit_linter.py

@@ -611,6 +626,8 @@
 _STDOUT_LIST = multiprocessing.Manager().list()
 _FILES = multiprocessing.Manager().dict()

+MAX_CONCURRENT_RUNS = 6


This is more of a global number, I'd recommend putting this in the semaphore_method file. The max is whatever is in the original backend test file.

kevinlee12 · 2020-02-29T20:26:56Z

scripts/pre_commit_linter.py

+        all_messages += _join_linting_process(
+            linting_processes, result_queues, stdout_queues)


This is going to kick off all of the linting processes at once, which is what you don't want to do.

kevinlee12 · 2020-02-29T20:31:44Z

scripts/pre_commit_linter.py

-    """This function is used to check if node-eslint dependencies are
-    installed and pass ESLint binary path and lint all the files(JS, Python,
-    HTML, CSS) with their respective third party linters.
+class LintingTaskSpec(python_utils.OBJECT):


This isn't exactly what we're looking for, since the code here is essentially repackaging the old method. Instead, you want to is to break out each linter (Python, JS, etc) into a separate check like PythonLintChecksManager (but for Pylint). That way, you'll have a list of tasks for each linter operation that you'll pass in.

I have made some changes. PTAL! If they are alright.

It's heading in the right direction, but I'd recommend pulling out the third party linters into it's own Manager class, that way there's even more isolation.

Also, because these third party libraries have their own ways to lint at their fastest (ie. might parallelize on their own), I'd recommend actually having 2 queues. One queue for the third party linters like Pylint and ESLint where the Semaphore number is 1 and another for the custom linting functions where the semaphore number is the default.

If you like, you can probably pull out the linters into separate files, and keep this module for just the logic that is common to all linters. (You might not need a manager class in that case.)

@kevinlee12 so you want me to bring _lint_py_files under a Class ThirdPartyPythonLIntChecks and same for all other third party linters?

They should be in separate classes.

@seanlip Maybe because this file contains BAD_PATTERNS and MANDATORY_PATTERNS definitions which is used by almost all the linters.

You can have the bad patterns and mandatory patterns check in this file, and separate all the other filetype-specific lint checks out. Then, what would happen is: the general lint checks in this file run always, and then the specific ones run for the given input filetype. Would that work?

@seanlip we also have EXCLUDED_PATHS variable and redirect_stdout method and many other global variables like FILES, FILE_CACHE, etc. which are used by every lint check. If we separate all checks then we have to define these in every file.

It seems like a good exercise to try and figure out what the dependencies actually are and refactor -- having everything in the same place makes it more likely that you get a "big blob of mud".

Stuff like EXCLUDED_PATHS falls in the "just do this check before passing files to the sub-linters" category. I'm not sure what FILES and FILE_CACHE are used for but it may be a good idea to figure that out and define an interface for what the seam between the main linter and the sub-linters is. Having the sub-linters be in separate files will help with this as it will require you to understand the dependencies properly and find the correct "break" points.

kevinlee12 · 2020-02-29T20:32:23Z

scripts/semaphore_method.py

@@ -0,0 +1,128 @@
+# coding: utf-8


Rename this to semaphore_utils instead.

Hudda · 2020-03-22T05:35:12Z

OK -- to be clearer, this is how the whole Returns part should look (here and elsewhere):
tuple(None, ThirdPartyCSSLintChecksManager). A 2-tuple of custom and 
third_party linter objects.
Note: no "Returns", and no need to indent the second line.

Done. @seanlip ptal

seanlip · 2020-03-22T05:37:32Z

Thanks, no further comments from my end!

Please get reviews from @vojtechjelinek and @nithusha21.

Hudda · 2020-03-22T05:38:21Z

Thanks, no further comments from my end!

Please get reviews from @vojtechjelinek and @nithusha21.

Thanks @seanlip

nithusha21

LGTM for the codeowner file (core/tests)

nithusha21 · 2020-03-22T05:47:03Z

Just one note, some wiki pages would need to be updated with the new script command.

Hudda · 2020-03-22T07:29:10Z

Just one note, some wiki pages would need to be updated with the new script command.

Thanks, @nithusha21 I will update the wiki once this PR gets merged.

vojtechjelinek

LGTM from code owner's perspective. Thanks!

bansalnitish · 2020-03-22T15:30:53Z

Please do not merge, I want to take a pass over this (given this is a part of the linter project).

kevinlee12 · 2020-03-22T22:35:24Z

@bansalnitish please do expedite the review since there is large interest in the community to make new linter checks. thanks!

bansalnitish

LGTM!

…ppia#8656) * Splited linter by file extension types * Run linter using semaphore method * Used Semaphore method for linting * Fix yarn.lock * Added more isolation * Fix yarn.lock * Added manager for third party lint checks * Fix typo * Splitted the linter into sub linters * Moved under linter directory * Addressed review changes * Changed pre_commit_linter path * Temporarily disabled BlankLineBelowFileOverviewChecker * Fixed lint error * Added test for redirect_stdout * Removed html directive check * Addressed review comments * Used list.extend * Fixed lint errors * Addressed review comments * Increased concurrency limit to 2 * Addressed review comments * Fixed lint errors * Semaphore fixed * Addressed review comments * Addressed review comments. * Addressed review comments * Addressed review comments * Addressed review comments

nithusha21 · 2020-04-04T15:34:16Z

Just following up.. @Hudda did you update the wiki pages? If so, can you link them here?

Hudda · 2020-04-05T05:42:47Z

@nithusha21 Here are the links to wiki pages: FAQ, Coding style guide

nithusha21 · 2020-04-05T06:16:40Z

Thanks!

DubeySandeep · 2020-04-15T11:19:14Z

scripts/linters/pre_commit_linter.py

+        all_file_extensions_type: list(str). The list of all file extensions
+        to be linted and checked.
+    """
+    all_file_extensions_type = ['js', 'py', 'html', 'css', 'other']


@Hudda, Why ts is not added here?

@DubeySandeep we do not have separate linter for js and ts, if both options used together then Js/Ts linter will run twice.

So how the general-purpose check would run for ts files?

@DubeySandeep Thanks for pointing this out. I will open a PR to fix this issue.

Can you please resolve it asap! (We have an open issue for the same at #8993 and I've assigned you to the same.)

(Note: This is a critical issue, a PR can be merged with fit and this can cause CI to run only one test.)

DubeySandeep · 2020-04-15T11:20:40Z

scripts/linters/pre_commit_linter.py

+                'are used together, then the JS/TS linter will be run twice.')
+            python_utils.PRINT('Exiting...')
+            sys.exit(1)
+


@Hudda Do we need to check whether all the passes params in file_extensions_to_lint are valid?

@DubeySandeep validation is added here. This option in argparse only allows a limited subset of choices.

Splited linter by file extension types

914dc1c

Hudda requested a review from kevinlee12 as a code owner February 16, 2020 17:21

Merge remote-tracking branch 'upstream/develop' into split-linter

424a318

Hudda mentioned this pull request Feb 16, 2020

Migration of lint checks to Code Climate #8267

Closed

9 tasks

Hudda assigned kevinlee12 and seanlip Feb 16, 2020

Hudda added the PR CHANGELOG: Developer experience -- @kevinlee12 label Feb 16, 2020

kevinlee12 suggested changes Feb 17, 2020

View reviewed changes

kevinlee12 assigned Hudda and unassigned kevinlee12 and seanlip Feb 17, 2020

oppiabot bot added stale and removed stale labels Feb 24, 2020

Hudda added 4 commits February 29, 2020 16:52

Run linter using semaphore method

83f899e

Merge remote-tracking branch 'upstream/develop' into split-linter

016d7b8

Used Semaphore method for linting

5c67a15

Fix yarn.lock

03684cc

Hudda assigned kevinlee12 and unassigned Hudda Feb 29, 2020

kevinlee12 reviewed Feb 29, 2020

View reviewed changes

kevinlee12 assigned Hudda and unassigned kevinlee12 Feb 29, 2020

Hudda added 2 commits March 1, 2020 23:45

Added more isolation

a6c117f

Fix yarn.lock

fc20a69

Hudda removed their assignment Mar 1, 2020

seanlip assigned Hudda Mar 22, 2020

Addressed review comments

de75904

Hudda assigned seanlip and unassigned Hudda Mar 22, 2020

seanlip removed their assignment Mar 22, 2020

nithusha21 approved these changes Mar 22, 2020

View reviewed changes

nithusha21 assigned Hudda and unassigned nithusha21 Mar 22, 2020

Hudda removed their assignment Mar 22, 2020

vojtechjelinek approved these changes Mar 22, 2020

View reviewed changes

bansalnitish assigned bansalnitish and unassigned vojtechjelinek Mar 22, 2020

bansalnitish approved these changes Mar 23, 2020

View reviewed changes

bansalnitish merged commit 8c37b1e into oppia:develop Mar 23, 2020

Hudda deleted the split-linter branch March 23, 2020 08:48

ankita240796 mentioned this pull request Mar 28, 2020

Lint error while pre commit linter runs #8935

Closed

DubeySandeep reviewed Apr 15, 2020

View reviewed changes

DubeySandeep mentioned this pull request Apr 15, 2020

The linter test is not catching errors of the bad pattern js and ts regular expressions #8993

Closed

		all_messages += _join_linting_process(
		linting_processes, result_queues, stdout_queues)

Fixes #8625: Split linters to run for a specific file extension #8656

Fixes #8625: Split linters to run for a specific file extension #8656

Conversation

Hudda commented Feb 16, 2020 • edited Loading

Explanation

Checklist

oppiabot bot commented Feb 16, 2020

oppiabot bot commented Feb 16, 2020

kevinlee12 left a comment

Choose a reason for hiding this comment

seanlip commented Feb 17, 2020 • edited Loading

oppiabot bot commented Feb 24, 2020

Hudda commented Feb 29, 2020 • edited Loading

kevinlee12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hudda Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hudda commented Mar 22, 2020

seanlip commented Mar 22, 2020

Hudda commented Mar 22, 2020

nithusha21 left a comment

Choose a reason for hiding this comment

nithusha21 commented Mar 22, 2020

Hudda commented Mar 22, 2020

vojtechjelinek left a comment

Choose a reason for hiding this comment

bansalnitish commented Mar 22, 2020

kevinlee12 commented Mar 22, 2020

bansalnitish left a comment

Choose a reason for hiding this comment

nithusha21 commented Apr 4, 2020

Hudda commented Apr 5, 2020 • edited Loading

nithusha21 commented Apr 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hudda commented Feb 16, 2020 •

edited

Loading

seanlip commented Feb 17, 2020 •

edited

Loading

Hudda commented Feb 29, 2020 •

edited

Loading

Hudda Mar 4, 2020 •

edited

Loading

Hudda commented Apr 5, 2020 •

edited

Loading