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

[core] Use turborepo to run the typescript task #9928

Draft
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

cherniavskii
Copy link
Member

We have a lot of packages and two independent teams in MUI X, it's time to start optimizing our workflow a bit πŸ™‚

I gave Turborepo a try and started using it for the typescript task.
The main goal is to avoid compiling packages that have not changed. For example, when working on the data grid, the other packages will not be compiled unless their files change.

If it works well, we can use it for other tasks. I believe I can integrate it with the test:karma task (I usually use it to test everything locally).

We don't use Remote caching yet, which means that each team member will have their own local cache. We can look into adding the remote cache later to speed up our CI.


For comparison, after I change a file in the data grid package, yarn typescript takes ~8s πŸš€:

yarn typescript
yarn run v1.22.17
$ turbo run --parallel --continue typescript
β€’ Packages in scope: @mui/x-charts, @mui/x-codemod, @mui/x-data-grid, @mui/x-data-grid-generator, @mui/x-data-grid-premium, @mui/x-data-grid-pro, @mui/x-date-pickers, @mui/x-date-pickers-pro, @mui/x-license-pro, @mui/x-tree-view, docs, eslint-plugin-material-ui
β€’ Running typescript in 12 packages
β€’ Remote caching disabled
docs:typescript: cache hit (outputs already on disk), replaying logs 750b65f6100baa8c
@mui/x-tree-view:typescript: cache hit (outputs already on disk), replaying logs 9db66a35e783e5d3
@mui/x-codemod:typescript: cache hit (outputs already on disk), replaying logs d16ab5fdc4cda081
@mui/x-date-pickers:typescript: cache hit (outputs already on disk), replaying logs b43dc4375c0637c3
@mui/x-data-grid-premium:typescript: cache hit (outputs already on disk), replaying logs 4c690cb12e94a213
@mui/x-data-grid-generator:typescript: cache hit (outputs already on disk), replaying logs a5a5b89a02f84db0
@mui/x-license-pro:typescript: cache hit (outputs already on disk), replaying logs b48cef00597aa62d
@mui/x-data-grid:typescript: cache miss, executing 7afa33b1c652eb9d
docs:typescript: $ tsc -p tsconfig.json
@mui/x-date-pickers:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid-premium:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid-generator:typescript: $ tsc -p tsconfig.json
@mui/x-license-pro:typescript: $ tsc -p tsconfig.json
@mui/x-codemod:typescript: $ tsc -p tsconfig.json
@mui/x-tree-view:typescript: $ tsc -p tsconfig.json
@mui/x-date-pickers-pro:typescript: cache hit (outputs already on disk), replaying logs 99b340f6eb0dec4f
@mui/x-data-grid-pro:typescript: cache hit (outputs already on disk), replaying logs 09760b702dee4d3c
@mui/x-charts:typescript: cache hit (outputs already on disk), replaying logs a585dd3aef2f65af
@mui/x-data-grid-pro:typescript: $ tsc -p tsconfig.json
@mui/x-date-pickers-pro:typescript: $ tsc -p tsconfig.json
@mui/x-charts:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid:typescript: $ tsc -p tsconfig.json

 Tasks:    11 successful, 11 total
Cached:    10 cached, 11 total
  Time:    7.862s 

✨  Done in 8.55s.

When bypassing the cache, it takes ~42s:

yarn typescript        
yarn run v1.22.17
$ turbo run --parallel --continue typescript
β€’ Packages in scope: @mui/x-charts, @mui/x-codemod, @mui/x-data-grid, @mui/x-data-grid-generator, @mui/x-data-grid-premium, @mui/x-data-grid-pro, @mui/x-date-pickers, @mui/x-date-pickers-pro, @mui/x-license-pro, @mui/x-tree-view, docs, eslint-plugin-material-ui
β€’ Running typescript in 12 packages
β€’ Remote caching disabled
@mui/x-license-pro:typescript: cache hit (outputs already on disk), replaying logs b48cef00597aa62d
docs:typescript: cache hit (outputs already on disk), replaying logs 750b65f6100baa8c
@mui/x-date-pickers:typescript: cache hit (outputs already on disk), replaying logs b43dc4375c0637c3
@mui/x-date-pickers:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid:typescript: cache hit (outputs already on disk), replaying logs 7a62fe8f89574c44
@mui/x-data-grid-premium:typescript: cache hit (outputs already on disk), replaying logs 4c690cb12e94a213
@mui/x-data-grid:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid-pro:typescript: cache hit (outputs already on disk), replaying logs 09760b702dee4d3c
docs:typescript: $ tsc -p tsconfig.json
@mui/x-charts:typescript: cache hit (outputs already on disk), replaying logs a585dd3aef2f65af
@mui/x-data-grid-pro:typescript: $ tsc -p tsconfig.json
@mui/x-license-pro:typescript: $ tsc -p tsconfig.json
@mui/x-date-pickers-pro:typescript: cache hit (outputs already on disk), replaying logs 99b340f6eb0dec4f
@mui/x-date-pickers-pro:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid-premium:typescript: $ tsc -p tsconfig.json
@mui/x-charts:typescript: $ tsc -p tsconfig.json
@mui/x-codemod:typescript: cache hit, replaying logs d16ab5fdc4cda081
@mui/x-tree-view:typescript: cache hit, replaying logs 9db66a35e783e5d3
@mui/x-tree-view:typescript: $ tsc -p tsconfig.json
@mui/x-codemod:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid-generator:typescript: cache hit, replaying logs a5a5b89a02f84db0
@mui/x-data-grid-generator:typescript: $ tsc -p tsconfig.json

 Tasks:    11 successful, 11 total
Cached:    11 cached, 11 total
  Time:    228ms >>> FULL TURBO

✨  Done in 0.73s.
➜  mui-x git:(turborepo) yarn typescript --force
yarn run v1.22.17
$ turbo run --parallel --continue typescript --force
β€’ Packages in scope: @mui/x-charts, @mui/x-codemod, @mui/x-data-grid, @mui/x-data-grid-generator, @mui/x-data-grid-premium, @mui/x-data-grid-pro, @mui/x-date-pickers, @mui/x-date-pickers-pro, @mui/x-license-pro, @mui/x-tree-view, docs, eslint-plugin-material-ui
β€’ Running typescript in 12 packages
β€’ Remote caching disabled
@mui/x-data-grid-premium:typescript: cache bypass, force executing 4c690cb12e94a213
@mui/x-date-pickers-pro:typescript: cache bypass, force executing 99b340f6eb0dec4f
@mui/x-data-grid:typescript: cache bypass, force executing 7a62fe8f89574c44
@mui/x-data-grid-generator:typescript: cache bypass, force executing a5a5b89a02f84db0
@mui/x-tree-view:typescript: cache bypass, force executing 9db66a35e783e5d3
@mui/x-codemod:typescript: cache bypass, force executing d16ab5fdc4cda081
@mui/x-data-grid-pro:typescript: cache bypass, force executing 09760b702dee4d3c
@mui/x-license-pro:typescript: cache bypass, force executing b48cef00597aa62d
@mui/x-charts:typescript: cache bypass, force executing a585dd3aef2f65af
docs:typescript: cache bypass, force executing 750b65f6100baa8c
@mui/x-date-pickers:typescript: cache bypass, force executing b43dc4375c0637c3
docs:typescript: $ tsc -p tsconfig.json
@mui/x-codemod:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid-generator:typescript: $ tsc -p tsconfig.json
@mui/x-charts:typescript: $ tsc -p tsconfig.json
@mui/x-date-pickers-pro:typescript: $ tsc -p tsconfig.json
@mui/x-date-pickers:typescript: $ tsc -p tsconfig.json
@mui/x-license-pro:typescript: $ tsc -p tsconfig.json
@mui/x-tree-view:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid-premium:typescript: $ tsc -p tsconfig.json
@mui/x-data-grid-pro:typescript: $ tsc -p tsconfig.json

 Tasks:    11 successful, 11 total
Cached:    0 cached, 11 total
  Time:    41.861s 

✨  Done in 42.38s.

@cherniavskii cherniavskii added the core Infrastructure work going on behind the scenes label Aug 4, 2023
@mui-bot
Copy link

mui-bot commented Aug 4, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9928--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean Οƒ
Filter 100k rows ms 316.3 500.5 386.8 385.18 66.229
Sort 100k rows ms 705.7 1,331 1,270.9 1,127.2 227.486
Select 100k rows ms 504.7 784.1 672.5 665.04 93.665
Deselect 100k rows ms 135.8 273.5 191.3 192.82 50.982

Generated by 🚫 dangerJS against 1843b33

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 7, 2023

Interesting, ideas:

  1. I think we should see how this impact the CI. How could Turborepo know all the dependencies that can change the type linting outcome? Should we trust the cache for the CI?
  2. I'm curious, What's the use case for running "yarn typescript" locally, with all the codebase? My workflow so far is mostly pushed to CI, waiting for it to fail, opening VS Code, and fixing the error in VS Code. I ran it locally when I needed to iterate on global types, impacting all codebase.
  3. Why Turborepo, how does it compare to Nx? https://nx.dev/concepts/more-concepts/turbo-and-nx

@flaviendelangle
Copy link
Member

My workflow so far is mostly pushed to CI, waiting for it to fail,

That's not very optimized to limit CI costs πŸ˜†

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 8, 2023

CI costs

Actually, going off topic of this PR, I think I can bring this up, in case there are applications for this: we spend about $40/month/engineer today, $1,000/month in total. I think we could trade spending x2 more for a x2 better CI experience πŸ€—. This is also equivalent to say spending the same monthly amount but having someone full time for a month per year to optimize the MUI CI.

@flaviendelangle
Copy link
Member

We talked about the amount of CI launched when talking about Argos a few weeks back.
Does that mean that the amount of run in Argos is not limiting either?

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 14, 2023

Does that mean that the amount of run in Argos is not limiting either?

Pretty much. Today, the only limit I'm aware of is: the more screenshots we push the longer it takes to get the answer.
I doubt there will ever be run limits with Argos CI since down the self-hosting route, the cost is mostly setup-related, not so much running the infra.

@cherniavskii
Copy link
Member Author

  1. I think we should see how this impact the CI. How could Turborepo know all the dependencies that can change the type linting outcome? Should we trust the cache for the CI?

We can configure cache inputs - see https://turbo.build/repo/docs/core-concepts/caching/file-inputs
In the case of this PR, I've added top-level tsconfig as a global dependency of each package, because each package extends it - https://github.com/mui/mui-x/pull/9928/files#diff-f8de965273949793edc0fbfe249bb458c0becde39b2e141db087bcbf5d4ad5e3R3
Should we trust the cache for the CI? Well, in case of misconfiguration, we might experience false positives πŸ€·πŸ»β€β™‚οΈ
So the CI speedup might not be worth the risk.

  1. I'm curious, What's the use case for running "yarn typescript" locally, with all the codebase? My workflow so far is mostly pushed to CI, waiting for it to fail, opening VS Code, and fixing the error in VS Code. I ran it locally when I needed to iterate on global types, impacting all codebase.

I usually run it locally before opening the PR to quickly make sure I didn't miss anything.
When making a change that would make TS fail, I run it to see where it breaks.
And each time it unnecessarily compiles date pickers and charts.
Alternatively, we can use lerna's --scope flag: yarn typescript --scope @mui/x-data-grid --scope @mui/x-data-grid-pro --scope @mui/x-data-grid-premium @mui/x-data-grid-generator

  1. Why Turborepo, how does it compare to Nx?

I didn't know about caching available in Nx, will take a look πŸ‘

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 31, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2023

The main goal is to avoid compiling packages that have not changed.

Did we consider this approach? https://github.com/mui/material-ui/blob/b2c4a8607e0368cfee2bc5ec21aefdc6a0390ed6/.circleci/config.yml#L159-L174.


When it comes to having a faster CI, a few ideas looking at https://app.circleci.com/pipelines/github/mui/mui-x/43569/workflows/e7badba4-3576-41cb-a640-2fd7a26fd345

  • pnpm over yarn could both speedup install time and lower caching space needed for dependencies.
  • test_unit is quite slow. 10 minutes to run. I think we should look at these:
  • in "Run visual regression tests" do we really need to take 1,000 screenshots? Even if Argos CI isn't the bottleneck, taking those can be not so fast.

@Janpot
Copy link
Member

Janpot commented Sep 4, 2023

  • in "Run visual regression tests" do we really need to take 1,000 screenshots? Even if Argos CI isn't the bottleneck, taking those can be not so fast.

The visual regression tests run in half the time as the unit tests, don't rely on any implementation details, and test direct user impact. They cover so much ground, if anything, I think we should have more of them πŸ˜„

@flaviendelangle
Copy link
Member

The problem with the visual regression test is that for some set of components, we have dozens of demos that looks identical before interaction (pickers being a good example).
So those are pretty much useless.
But I do agree that if we can add a visual regression test that do not exist yet, it's almost always interesting.

@Janpot
Copy link
Member

Janpot commented Sep 4, 2023

The problem with the visual regression test is that for some set of components, we have dozens of demos that looks identical before interaction (pickers being a good example).

I see. One thing to note is that it's not because the screenshots look the same that they are necessarily testing the same thing. One could argue they are testing whether a different configuration affects the initial appearance. You'll have better context about this than me though to judge whether this applies and is valuable enough to justify the overhead.

@flaviendelangle
Copy link
Member

One thing to note is that it's not because the screenshots look the same that they are necessarily testing the same thing. One could argue they are testing whether a different configuration affects the initial appearance.

For the pickers, the input are looking the same across a large set of demos.
And it's because nothing varies across those demos for the input, the varying factor is inside the view.
So for them, I think it's pure duplicate and useless screenshot.
But I'd be okay to say that, it represents a small fraction of the overall amount of screenshots and therefore it's not worth trying to filter them agressively.

@michelengelen michelengelen changed the base branch from master to next November 6, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants