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

Composable turbo.json #2706

Merged
merged 190 commits into from
Feb 13, 2023
Merged

Composable turbo.json #2706

merged 190 commits into from
Feb 13, 2023

Conversation

mehulkar
Copy link
Contributor

@mehulkar mehulkar commented Nov 15, 2022

This feature allows monorepos to place a turbo.json file in workspaces and add a "extends" key to extend from the root configuration to add/remove to the pipeline. After this PR, individual workspaces should be able to:

  • add keys to task defintiions (e.g. changing dependsOn or outputs configs)
  • reset keys in task definitions to defaults (e.g. dependsOn: []`)
  • add tasks (e.g add a lint command only in the workspace that implements it)
Limitations

Some of these limitations are edge cases, but still listing them out. You cannot:

  • extend config other than the pipeline tasks. For example, remoteCache, globalEnv, etc cannot be changed for individual workspaces.
  • extend from other workspaces.
  • configure the root turbo.json to extend from anything.
  • add or subtract to existing array configs (e.g. if your root config has dependsOn: ['something'], you can't append to this array, you can only overwrite it.)
  • declare tasks in the package#task syntax in workspaces. The package is inferred.

These limitations are based on a lot of research to keep the design space open, gather use cases, and avoid making the wrong decisions before putting the feature into the hands of real users. In short: to make it possible to ship this sooner!

Example
  • ./turbo.json (root)

     {
     "pipeline": {
     		"build": {
     			"outputs": ["dist/**"],
     			"dependsOn": ["lint"]
     		}
     	}
     }
  • ./packages/ui/turbo.json (ui workspace)

     {
     	"extends": ["//"],
     	"pipeline": {
     		"build": {
     			"dependsOn": ["lint", "compile"],
     			"outputs": ["lib/**"]
     		}
     	}
     }

With these configs, when you run turbo build, all workspaces except the ui workspace will cache the dist/ directory and run their lint commands first. The ui workspace will run lint and compile first, and cache the lib/ directory instead.

Review Notes
  • Implementation is not super robust, it just looks for a workspace config and root config and merges them. I'd like to get this out as feature-complete work, before adding any crawling behavior for future work.
  • Read the test cases (*.t files) to see what use cases this supports
  • Happy to pair-review anything.
Tests
  • When workspace adds a new task that is not defined in the root config
  • When workspace adds new keys into task config that are not defined in the root
  • When workspace has a task definition, but omits some keys that root config has
  • When workspace overrides individual keys of a task config
  • When workspace doesn't have a turbo.json
Pre-req work
Deferred Work
  • All limitations noted above
  • Docs
  • Perf optimization: avoid reading workspace configs if we know they don't exist (e.g. do a glob find up front)
  • fieldsMeta + Merge is won't scale as new keys are added. Need a more elegant solution

@vercel
Copy link

vercel bot commented Nov 15, 2022

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

Name Status Preview Comments Updated
examples-svelte-web 🔄 Building (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
turbo-site 🔄 Building (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
9 Ignored Deployments
Name Status Preview Comments Updated
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
examples-cra-web ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
examples-nonmonorepo ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 9:07PM (UTC)
turbo-vite-web ⬜️ Ignored (Inspect) Feb 13, 2023 at 9:07PM (UTC)

@github-actions
Copy link
Contributor

github-actions bot commented Nov 15, 2022

🟢 CI successful 🟢

Thanks

@mehulkar mehulkar force-pushed the mehulkar/turbo-538-composable-turbojson branch from 7607a0f to 533d0f0 Compare November 17, 2022 00:57
@mehulkar mehulkar changed the base branch from main to mk/topo-graphs November 17, 2022 00:57
@mehulkar mehulkar changed the title lets get this started Composable turbo.json Nov 17, 2022
@mehulkar mehulkar force-pushed the mehulkar/turbo-538-composable-turbojson branch from c10c116 to 5cab00b Compare November 17, 2022 23:13
Base automatically changed from mk/topo-graphs to main November 17, 2022 23:46
@mehulkar mehulkar force-pushed the mehulkar/turbo-538-composable-turbojson branch from 5cab00b to 1525aee Compare November 17, 2022 23:50
@mehulkar mehulkar removed area: docs Improvements or additions to documentation area: site labels Jan 11, 2023
@mehulkar mehulkar force-pushed the mehulkar/turbo-538-composable-turbojson branch from 0333956 to 3fcffe1 Compare January 12, 2023 07:56
@mehulkar mehulkar force-pushed the mehulkar/turbo-538-composable-turbojson branch 2 times, most recently from 47dbfaa to 79a63da Compare January 18, 2023 03:37
@mehulkar mehulkar force-pushed the mehulkar/turbo-538-composable-turbojson branch 3 times, most recently from f0ac23c to e9fd2ad Compare January 18, 2023 07:56
Copy link
Contributor

@nathanhammond nathanhammond left a comment

Choose a reason for hiding this comment

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

Integration tests could, in many cases, remove possible future failures by using a bit of jq.

(I know it's a different code path under the hood, so I don't know if that was intentional, thus a comment.)

Comment on lines 58 to 100
# 3. Change input file and assert cache miss
$ echo "more text" >> $TARGET_DIR/apps/add-keys/src/foo.txt
$ ${TURBO} run add-keys-task --filter=add-keys
\xe2\x80\xa2 Packages in scope: add-keys (esc)
\xe2\x80\xa2 Running add-keys-task in 1 packages (esc)
\xe2\x80\xa2 Remote caching disabled (esc)
add-keys:add-keys-underlying-task: cache miss, executing b7390d43fae5a180
add-keys:add-keys-underlying-task:
add-keys:add-keys-underlying-task: > add-keys-underlying-task
add-keys:add-keys-underlying-task: > echo "running add-keys-underlying-task"
add-keys:add-keys-underlying-task:
add-keys:add-keys-underlying-task: running add-keys-underlying-task
add-keys:add-keys-task: cache miss, executing b256f31e3957fee3
add-keys:add-keys-task:
add-keys:add-keys-task: > add-keys-task
add-keys:add-keys-task: > echo "running add-keys-task" > out/foo.min.txt
add-keys:add-keys-task:

Tasks: 2 successful, 2 total
Cached: 0 cached, 2 total
Time:\s*[\.0-9]+m?s (re)

# 4. Set env var and assert cache miss
$ SOME_VAR=somevalue ${TURBO} run add-keys-task --filter=add-keys
\xe2\x80\xa2 Packages in scope: add-keys (esc)
\xe2\x80\xa2 Running add-keys-task in 1 packages (esc)
\xe2\x80\xa2 Remote caching disabled (esc)
add-keys:add-keys-underlying-task: cache hit, replaying output b7390d43fae5a180
add-keys:add-keys-underlying-task:
add-keys:add-keys-underlying-task: > add-keys-underlying-task
add-keys:add-keys-underlying-task: > echo "running add-keys-underlying-task"
add-keys:add-keys-underlying-task:
add-keys:add-keys-underlying-task: running add-keys-underlying-task
add-keys:add-keys-task: cache miss, executing 8e76bee49319e00e
add-keys:add-keys-task:
add-keys:add-keys-task: > add-keys-task
add-keys:add-keys-task: > echo "running add-keys-task" > out/foo.min.txt
add-keys:add-keys-task:

Tasks: 2 successful, 2 total
Cached: 1 cached, 2 total
Time:\s*[\.0-9]+m?s (re)

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we used dry-run to reduce visual complexity for non-output-related tests? We're testing the hashes here, not the log messages.

Suggested change
# 3. Change input file and assert cache miss
$ echo "more text" >> $TARGET_DIR/apps/add-keys/src/foo.txt
$ ${TURBO} run add-keys-task --filter=add-keys
\xe2\x80\xa2 Packages in scope: add-keys (esc)
\xe2\x80\xa2 Running add-keys-task in 1 packages (esc)
\xe2\x80\xa2 Remote caching disabled (esc)
add-keys:add-keys-underlying-task: cache miss, executing b7390d43fae5a180
add-keys:add-keys-underlying-task:
add-keys:add-keys-underlying-task: > add-keys-underlying-task
add-keys:add-keys-underlying-task: > echo "running add-keys-underlying-task"
add-keys:add-keys-underlying-task:
add-keys:add-keys-underlying-task: running add-keys-underlying-task
add-keys:add-keys-task: cache miss, executing b256f31e3957fee3
add-keys:add-keys-task:
add-keys:add-keys-task: > add-keys-task
add-keys:add-keys-task: > echo "running add-keys-task" > out/foo.min.txt
add-keys:add-keys-task:
Tasks: 2 successful, 2 total
Cached: 0 cached, 2 total
Time:\s*[\.0-9]+m?s (re)
# 4. Set env var and assert cache miss
$ SOME_VAR=somevalue ${TURBO} run add-keys-task --filter=add-keys
\xe2\x80\xa2 Packages in scope: add-keys (esc)
\xe2\x80\xa2 Running add-keys-task in 1 packages (esc)
\xe2\x80\xa2 Remote caching disabled (esc)
add-keys:add-keys-underlying-task: cache hit, replaying output b7390d43fae5a180
add-keys:add-keys-underlying-task:
add-keys:add-keys-underlying-task: > add-keys-underlying-task
add-keys:add-keys-underlying-task: > echo "running add-keys-underlying-task"
add-keys:add-keys-underlying-task:
add-keys:add-keys-underlying-task: running add-keys-underlying-task
add-keys:add-keys-task: cache miss, executing 8e76bee49319e00e
add-keys:add-keys-task:
add-keys:add-keys-task: > add-keys-task
add-keys:add-keys-task: > echo "running add-keys-task" > out/foo.min.txt
add-keys:add-keys-task:
Tasks: 2 successful, 2 total
Cached: 1 cached, 2 total
Time:\s*[\.0-9]+m?s (re)
# 3. Change input file and assert cache miss
$ echo "more text" >> $TARGET_DIR/apps/add-keys/src/foo.txt
$ ${TURBO} run add-keys-task --filter=add-keys --dry=json | jq -r ".tasks[].hash"
b7390d43fae5a180
b256f31e3957fee3
# 4. Set env var and assert cache miss
$ SOME_VAR=somevalue ${TURBO} run add-keys-task --filter=add-keys --dry=json | jq -r ".tasks[].hash"
b7390d43fae5a180
8e76bee49319e00e

Comment on lines 5 to 18
$ ${TURBO} run added-task --filter=add-tasks
\xe2\x80\xa2 Packages in scope: add-tasks (esc)
\xe2\x80\xa2 Running added-task in 1 packages (esc)
\xe2\x80\xa2 Remote caching disabled (esc)
add-tasks:added-task: cache miss, executing 49e39f2b39207531
add-tasks:added-task:
add-tasks:added-task: > added-task
add-tasks:added-task: > echo "running added-task" > out/foo.min.txt
add-tasks:added-task:

Tasks: 1 successful, 1 total
Cached: 0 cached, 1 total
Time:\s+[.0-9]+m?s (re)

Copy link
Contributor

Choose a reason for hiding this comment

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

One bonus example:

Suggested change
$ ${TURBO} run added-task --filter=add-tasks
\xe2\x80\xa2 Packages in scope: add-tasks (esc)
\xe2\x80\xa2 Running added-task in 1 packages (esc)
\xe2\x80\xa2 Remote caching disabled (esc)
add-tasks:added-task: cache miss, executing 49e39f2b39207531
add-tasks:added-task:
add-tasks:added-task: > added-task
add-tasks:added-task: > echo "running added-task" > out/foo.min.txt
add-tasks:added-task:
Tasks: 1 successful, 1 total
Cached: 0 cached, 1 total
Time:\s+[.0-9]+m?s (re)
$ ${TURBO} run added-task --filter=add-tasks --dry=json | jq -r ".tasks[].hash"
49e39f2b39207531

$ HASH=$(cat tmp.log | grep -E "cached:cached-task-1.* executing .*" | awk '{print $5}')
$ echo $HASH
[a-z0-9]{16} (re)
$ tar -tf $TARGET_DIR/node_modules/.cache/turbo/$HASH.tar.zst;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use --cache-dir for these tests since many tests implicitly rely on that value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, (correct me if I'm wrong), we'd still have to do this dance to get the task hash. Is there any benefit to replacing$TARGET_DIR/node_modules/.cache/turbo with a custom value?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason is that it would make it possible to write a simple "check the result" helper function.

The "grab hash" thing is actually the thing I want to fix more than anything else, but we don't make that easy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll keep things the way they are, since they're already written, but I'll write new tests the way you suggest if applicable. E.g. #3778.

@mehulkar
Copy link
Contributor Author

Integration tests could, in many cases, remove possible future failures by using a bit of jq.

(I know it's a different code path under the hood, so I don't know if that was intentional, thus a comment.)

I'm open to this, but did it this way for a few reasons:

  • the code path is different as you mention
  • IMO we're a bit low on testing run and especially given the port, I think it would be good to have full "e2e" runs under many different scenarios.
  • I imagine that as we roll out this feature multiple configs will be a common case, so it makes sense to improve coverage in this purview, rather than, say integration_tests/basic_monorepo. In fact, composable_config/missing-workspace-config is essentially testing the current common scenario (no overrides).

I do agree that reading log output for the purpose of reading tests is hard though. Two things on that:

  • I could use --output-logs=hash-only when possible to reduce noise
  • Once the initial tests are written, it's not to bad to see diffs (especially in Github UI). And the cram VSCode extension makes things a lot easier to read also.

In any case, let me know what you think, I could see some of these test cases being reduced

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

I left a bunch of notes, but I think it nearly all boils down to one suggestion: at some level, either in fs or in graph, I think we should distinguish between a turbo file not being found vs some unexpected error occurring that needs to be reported to the user.

cli/internal/graph/graph.go Show resolved Hide resolved
cli/internal/graph/graph.go Outdated Show resolved Hide resolved
cli/internal/graph/graph.go Outdated Show resolved Hide resolved
cli/internal/core/engine.go Outdated Show resolved Hide resolved
cli/internal/core/engine.go Outdated Show resolved Hide resolved
cli/internal/core/engine.go Outdated Show resolved Hide resolved
mehulkar and others added 6 commits February 10, 2023 16:52
Adds a generic method to validate TurboJSON structs and validate
when getting the tasks.
Only allow the error where workspace config is missing, since
this config is not required. Throw other errors, including
when the contents of the workspace config are malformed.
This allows us to provide better error messaging to the end user
When `inputs` is configured for a task, we want to ensure that changes
to the `turbo.json` in that workspace cause a cache miss. This is
similar to how we treat the `package.json` in the workspace, and also
how we treat the root `turbo.json` for the global hash.
@mehulkar mehulkar merged commit 5b44c92 into main Feb 13, 2023
@mehulkar mehulkar deleted the mehulkar/turbo-538-composable-turbojson branch February 13, 2023 21:34
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.

6 participants