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

feat(core): run parallel based on the number of cpu cores #30543

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ianzone
Copy link

@ianzone ianzone commented Mar 30, 2025

BREAKING CHANGE: default parallel changed from 3 to half of the cpu cores

Current Behavior

can not specify parallel based on the number of cpu cores.

Expected Behavior

--parallel 0.5 means half of the cpu cores.
--parallel 3 means exact tasks to run in parallel.

Related Issue(s)

#30238

@ianzone ianzone requested a review from a team as a code owner March 30, 2025 11:32
@ianzone ianzone requested a review from MaxKless March 30, 2025 11:32
Copy link

vercel bot commented Mar 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Apr 8, 2025 8:53am

Copy link

nx-cloud bot commented Mar 31, 2025

View your CI Pipeline Execution ↗ for commit c3b6801.

Command Status Duration Result
nx-cloud record -- nx format:check --base=e29f8... ❌ Failed 2s View ↗
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded 30s View ↗
nx run-many -t check-imports check-commit check... ✅ Succeeded 15s View ↗
nx-cloud record -- nx-cloud conformance:check ✅ Succeeded 2s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded 1s View ↗
nx documentation ✅ Succeeded 1m 40s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-02 15:07:54 UTC

@MaxKless
Copy link
Collaborator

Hey @ianzone thanks for the PR!
Some points:

  • instead of a fractional number, I think it would make more sense to go with a percentage, like jest & vitest do it. It makes sense to align with other tools in order to be more intuitive. Then it's also clear that number specifies the cores directly while a string is percentage-based
  • Is there a reason that the default changed from 3 to 50%? I think it would be better to leave the default alone for now as it will affect everyone.

Here are some links to jest & vitest:

@ianzone
Copy link
Author

ianzone commented Mar 31, 2025

Hey @ianzone thanks for the PR! Some points:

  • instead of a fractional number, I think it would make more sense to go with a percentage, like jest & vitest do it. It makes sense to align with other tools in order to be more intuitive. Then it's also clear that number specifies the cores directly while a string is percentage-based
  • Is there a reason that the default changed from 3 to 50%? I think it would be better to leave the default alone for now as it will affect everyone.

Here are some links to jest & vitest:

  1. agree
  2. sometimes i'm kinda lazy and don't want to text extra words, and i guess 50% is a decent workload.

@MaxKless
Copy link
Collaborator

sure, I get it. But it's still a guess that will affect everyone and if the price is a couple of extra characters.... let's stick with the same default for now

@ianzone
Copy link
Author

ianzone commented Mar 31, 2025

sure, I get it. But it's still a guess that will affect everyone and if the price is a couple of extra characters.... let's stick with the same default for now

ok

@ianzone
Copy link
Author

ianzone commented Mar 31, 2025

updated

@MaxKless MaxKless self-assigned this Apr 1, 2025
@MaxKless
Copy link
Collaborator

MaxKless commented Apr 2, 2025

Looks good! Can you add a test to shared-options.spec.ts to verify this is and will keep behaving as it should? :) Thanks!

@ianzone
Copy link
Author

ianzone commented Apr 3, 2025

tests added

@ianzone
Copy link
Author

ianzone commented Apr 8, 2025

Hi, could you merge this?

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.

2 participants