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

Change Request: add ability to shard eslint #16726

Closed
1 task
gajus opened this issue Dec 29, 2022 · 37 comments
Closed
1 task

Change Request: add ability to shard eslint #16726

gajus opened this issue Dec 29, 2022 · 37 comments
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed Stale
Projects

Comments

@gajus
Copy link
Contributor

gajus commented Dec 29, 2022

ESLint version

Feature suggestion.

What problem do you want to solve?

Speed up ESLint execution by splitting the workload across multiple CPUs or multiple machines using sharding.

What do you think is the correct solution?

Copy the existing sharding behavior of libraries such as:

Basic principle of sharding: divide all matching files by the number of workerCount and only run linting against the files in the matching workerIndex.

Example:

Assuming these files

a.js
b.js
c.js
d.js
e.js
f.js

Then:

  • eslint --shard 1/3 would only lint a.js and b.js
  • eslint --shard 2/3 would only lint c.js and d.js
  • eslint --shard 3/3 would only lint e.js and f.js

This enables splitting ESLint workload either across multiple CPUs or multiple machines.

Participation

  • I am willing to submit a pull request for this change.

Additional comments

No response

@gajus gajus added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Dec 29, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Dec 29, 2022
@gajus
Copy link
Contributor Author

gajus commented Dec 29, 2022

Surprised I don't see prior discussion around this. Sorry if it was discussed using another term.

@fasttime
Copy link
Member

There is a similar issue: #3565

@gajus
Copy link
Contributor Author

gajus commented Dec 29, 2022

I am aware of that proposal. However, adding a --shard option is a lot simpler proposal.

@nzakas
Copy link
Member

nzakas commented Dec 29, 2022

"Copy what Jest does" isn't really a proposal we can evaluate. We've already been exploring linting files in parallel for a while, but likely won't revisit that until we do the larger rewrite.

If there's something specific about what Jest does that is different than what was already proposed in the RFC, please describe those differences on this issue.

@nzakas nzakas moved this from Needs Triage to Triaging in Triage Dec 29, 2022
@gajus gajus closed this as completed Dec 29, 2022
Triage automation moved this from Triaging to Complete Dec 29, 2022
@gajus
Copy link
Contributor Author

gajus commented Dec 29, 2022

Sharding and parallel linting are separate problems.

Assuming a directory:

foo.ts
bar.ts
baz.ts

In case of a shard eslint . --shard 1/3, eslint would only lint foo.ts file.

Sharding could be a precursor to parallel linting.

Parallelization is useful when you want to get the most of out multi-core machine. Sharding is useful when you want to batch the job. And because batches are predictable, it also allows to distribute the workload across multiple machines.

Once you have sharding, I would argue that ESLint doesn't even need native support for parallelization, as anyone can achieve that them themselves, e.g.

(eslint . --shard 1/3 & eslint . --shard 2/3 & eslint . --shard 3/3 &) wait;

@gajus
Copy link
Contributor Author

gajus commented Dec 30, 2022

@nzakas alternatively... if there was a way to pass a long list of files to ESLint, that would work too (sharding could be implemented in user land). However, my attempts to do that failed because of various limits. Maybe you have an idea how it is currently possible to pass a list of thousands of files to ESLint explicitly?

@bmish
Copy link
Sponsor Member

bmish commented Dec 30, 2022

Relatedly, I've had some success speeding up ESLint in a large codebase by running it on several large directories separately and in parallel instead of just running eslint . on everything:

{
  "scripts": {
    "lint:js": "npm-run-all --aggregate-output --continue-on-error --parallel \"lint:js:*\"",
    "lint:js:dir1": "eslint --cache dir1/",
    "lint:js:dir2": "eslint --cache dir2/",
    "lint:js:dir3": "eslint --cache dir3/",
    "lint:js:other": "eslint --cache --ignore-pattern dir1/ --ignore-pattern dir2/ --ignore-pattern dir3/ .",
  }
}

@gajus
Copy link
Contributor Author

gajus commented Dec 30, 2022

That's basically sharding, but limited by predefined patterns. This suggestion is a generic solution.

@gajus gajus reopened this Dec 30, 2022
Triage automation moved this from Complete to Evaluating Dec 30, 2022
@gajus
Copy link
Contributor Author

gajus commented Dec 30, 2022

Re-opening as I see a lot of value in having this.

@nzakas
Copy link
Member

nzakas commented Jan 2, 2023

Thanks for explaining. Just so I understand, if you pass --shard 1/3 to Jest right now, it just takes the total number of files, divides it by three, and only runs on that first third? So it's basically filtering out matches to the glob pattern?

Does Jest also do any parallelization or is it just sharding?

I'm not opposed to this, but I think it does come down to what we want ESLint to do at a high level. We've been bitten by adding a bunch of one-off enhancements before, only to find that they don't all make sense when added together. So, I'd want to have a good story about parallel linting vs. sharding vs. anything else before making a final decision on this.

And this would require an RFC to move forward. I think there are some edge cases (caching, fixing, maybe more) that would need some thought.

@nzakas nzakas moved this from Evaluating to Feedback Needed in Triage Jan 2, 2023
@gajus
Copy link
Contributor Author

gajus commented Jan 2, 2023

Thanks for explaining. Just so I understand, if you pass --shard 1/3 to Jest right now, it just takes the total number of files, divides it by three, and only runs on that first third? So it's basically filtering out matches to the glob pattern?

Correct.

Does Jest also do any parallelization or is it just sharding?

Jest runs parallelization on top of shard.

Parallelization is configured using --maxWorkers (defaults to the number of CPUs).

# runs all matching tests using 4 workers
jest --maxWorkers 4
# runs a third of matching tests
jest --shard 1/3
# runs a third of matching tests using 4 workers
jest --shard 1/3 --maxWorkers 4

For what it is worth, the same pattern is followed by Jest, Playwright, and Vitest.

Sharding can be implemented without parallelization and vice-versa. However, sharding is easier to implement and will produce better speed gains on large projects than parallelization (because there is only so much you can parallelize using cores on a single machine).

It is also worth noting that sharding can already be achieved using jest-runner-eslint in the exact same way as described above. The downside is that it attaches use of ESLint to Jest. In our case, we migrated away from Jest, so it is no longer an option.

I think there are some edge cases (caching, fixing, maybe more) that would need some thought.

I would not optimize sharding to be executed concurrently on the same machine. It can be, but I would not expect operations like cache to function. In my eyes, sharding is primarily designed to distribute workloads horizontally.

@nzakas
Copy link
Member

nzakas commented Jan 3, 2023

Thanks, that additional info is super helpful.

@nzakas nzakas added needs design Important details about this change need to be discussed and removed triage An ESLint team member will look at this issue soon labels Jan 3, 2023
@github-actions
Copy link

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Feb 12, 2023
@Rec0iL99
Copy link
Member

Rec0iL99 commented Feb 13, 2023

Waiting for an RFC. Don't close.

cc @gajus

@Rec0iL99 Rec0iL99 removed the Stale label Feb 13, 2023
@github-actions
Copy link

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Mar 15, 2023
@Rec0iL99
Copy link
Member

Sorry, won't get to this. Been pushing back on my TODO daily, but other priorities are getting in the way.

@gajus in that case, I'll close the issue. Feel free to re-open when you find the time to work on this. Thanks.

@Rec0iL99 Rec0iL99 closed this as not planned Won't fix, can't repro, duplicate, stale May 27, 2023
@nzakas
Copy link
Member

nzakas commented Jun 1, 2023

RFC was opened: eslint/rfcs#112

@balavishnuvj please be sure to comment on closed issues before opening PRs so we can keep up.

@nzakas nzakas reopened this Jun 1, 2023
@nzakas nzakas removed the Stale label Jun 1, 2023
@github-actions
Copy link

github-actions bot commented Jul 1, 2023

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Jul 1, 2023
@Rec0iL99
Copy link
Member

Rec0iL99 commented Jul 2, 2023

Not stale. RFC is open.

@Rec0iL99 Rec0iL99 removed the Stale label Jul 2, 2023
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Aug 1, 2023
@fasttime fasttime removed the Stale label Aug 2, 2023
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Sep 1, 2023
@nzakas
Copy link
Member

nzakas commented Sep 6, 2023

We have an RFC open for this, so not closing.

@Rec0iL99 Rec0iL99 removed the Stale label Sep 6, 2023
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Oct 6, 2023
@nzakas
Copy link
Member

nzakas commented Oct 9, 2023

RFC is open but hasn't been updated in a while. We likely will need someone to take over the RFC for this to progress.

@github-actions github-actions bot removed the Stale label Oct 9, 2023
Copy link

github-actions bot commented Nov 9, 2023

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Nov 9, 2023
@Rec0iL99
Copy link
Member

@nzakas should we close the RFC since there has been no response for a while now?

refer eslint/rfcs#112 (comment)

@github-actions github-actions bot removed the Stale label Nov 11, 2023
@nzakas
Copy link
Member

nzakas commented Nov 13, 2023

Yeah, just closed it, thanks for the heads up.

If anyone else is interested, we'd need an RFC to move forward with this.

Copy link

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Dec 13, 2023
@nzakas
Copy link
Member

nzakas commented Dec 14, 2023

Because there has been no movement on this in a while, I'm closing. If anyone is interested in resurrecting this proposal, please leave a comment and we'll reopen.

@nzakas nzakas closed this as not planned Won't fix, can't repro, duplicate, stale Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed Stale
Projects
Archived in project
Triage
Feedback Needed
Development

No branches or pull requests

7 participants
@nzakas @sam3k @bmish @gajus @fasttime @Rec0iL99 and others