Print resolvedTaskDefinition in DryRun #3377

mehulkar · 2023-01-19T08:40:10Z

This PR adds a resolvedTaskDefinition key to the Dry Run output. Today, this allows users to see the applied defaults for each key in their tasks. In the future, this will allow users to introspect the task config when it's composed from multiple sources.

write tests

vercel · 2023-01-19T08:40:21Z

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

10 Ignored Deployments

Name	Status	Preview	Updated
examples-basic-web	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)
examples-cra-web	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)
examples-designsystem-docs	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)
examples-kitchensink-blog	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)
examples-native-web	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)
examples-nonmonorepo	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)
examples-svelte-web	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)
examples-tailwind-web	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)
turbo-site	⬜️ Ignored (Inspect)	Visit Preview	Jan 20, 2023 at 2:34AM (UTC)
turbo-vite-web	⬜️ Ignored (Inspect)		Jan 20, 2023 at 2:34AM (UTC)

github-actions · 2023-01-19T08:47:18Z

🟢 CI successful 🟢

Thanks

mehulkar · 2023-01-19T21:22:40Z

cli/integration_tests/basic_monorepo/dry_run.t

+    Log File               = apps/my-app/.turbo/turbo-build.log                                                                
+    Dependencies           =                                                                                                   
+    Dependendents          =                                                                                                   
+    ResolvedTaskDefinition = {"outputs":[],"cache":true,"dependsOn":[],"inputs":[],"outputMode":0,"env":[],"persistent":false} 


I thought about not printing json here and trying have nested lines, but I think we want json, since it's trying to reflect user's input.

cli/integration_tests/basic_monorepo/dry_run.t

mehulkar · 2023-01-19T21:24:19Z

cli/internal/fs/turbo_json.go

-	OutputMode util.TaskOutputMode `json:"outputMode,omitempty"`
-	Env        []string            `json:"env,omitempty"`
-	Persistent bool                `json:"persistent,omitempty"`
+	Outputs    []string            `json:"outputs"`


changed this from a pointer, because I had trouble using it as a pointer in the new code, and I'm not sure if it needs to be one. I can change it back if it breaks something

A comment from #2712 that got deleted seems to indicating this was a pointer so it would get unmarshaled as nil instead of []string{}:

// We can't use omitempty for Outputs, because it will // always unmarshal into an empty array, which means something different from nil.

I think we don't need to differentiate between the two now that we don't supply default outputs.

I had understood that comment to be about omitempty, rather than the pointer (which is why i deleted the comment). Also that comment used to be in the Unmarshal method, rather than where the pointer was defined, so that seems to support the theory
https://github.com/vercel/turbo/blob/v1.6.0/cli/internal/fs/turbo_json.go#L215-L219

This is all correct, and a good change. Using weird Go implementation details to hack serialization is a smell, and I'm happy to have it gone.

mehulkar · 2023-01-19T21:27:39Z

cli/internal/run/dry_run.go

+	Dir                    string             `json:"directory"`
+	Dependencies           []string           `json:"dependencies"`
+	Dependents             []string           `json:"dependents"`
+	ResolvedTaskDefinition *fs.TaskDefinition `json:"resolvedTaskDefinition"`


Need to bikeshed the ResolvedTaskDefinition key name a bit.

My main concern is that "resolved" is misleading because we aren't going to include things like turbo's injected env vars or expand globs in inputs/outputs. I think that is ok, since this is a "task definition", and that "fully resolved state" is more for "cache hit status".

IMO I think the name pretty clear

mehulkar · 2023-01-19T21:27:58Z

cli/internal/run/dry_run.go

+	CacheState             cache.ItemStatus   `json:"cacheState"`
+	Command                string             `json:"command"`
+	Outputs                []string           `json:"outputs"`
+	ExcludedOutputs        []string           `json:"excludedOutputs"`


Outputs and ExcludedOutputs are redundant now, and I would like to remove them at some point in the future.

…in-dry-run

cli/integration_tests/basic_monorepo/dry_run.t

chris-olszewski · 2023-01-19T23:16:32Z

cli/internal/fs/turbo_json.go

-	OutputMode util.TaskOutputMode `json:"outputMode,omitempty"`
-	Env        []string            `json:"env,omitempty"`
-	Persistent bool                `json:"persistent,omitempty"`
+	Outputs    []string            `json:"outputs"`


A comment from #2712 that got deleted seems to indicating this was a pointer so it would get unmarshaled as nil instead of []string{}:

// We can't use omitempty for Outputs, because it will // always unmarshal into an empty array, which means something different from nil.

I think we don't need to differentiate between the two now that we don't supply default outputs.

cli/internal/run/dry_run.go

chris-olszewski · 2023-01-19T23:20:44Z

cli/internal/fs/turbo_json.go

@@ -301,6 +300,46 @@ func (c *TaskDefinition) UnmarshalJSON(data []byte) error {
 	return nil
 }

+// MarshalJSON deserializes JSON into a TaskDefinition
+func (c *TaskDefinition) MarshalJSON() ([]byte, error) {


Quick glancing through the code I think we aren't altering TaskDefinitions after construction, but since non-determinism has gotten us before: we might want to sort these slices before marshalling them. If you've done some closer reading of the code to verify that the slices remain sorted after creation, then feel free to ignore this comment.

Good call. That seems like a good idea, even just for determinism in tests, but I don't think it should affect task hashes, right? 🤔

Wrote a test first and it seemed to pass deterministically, but added a sort in anyway

I wonder how visible this change will be. I think it effectively changes the execution order of peer tasks?

None of that should be relied upon (and we should probably intentionally insert randomness into that at execution time to prevent people relying on it) but that doesn't mean people aren't implicitly relying on it right now.

(If we range over a map in our graph pass this is irrelevant, but I didn't check the stack trace.)

Based on my experience with test runs, the order is indeterminate already (see prysk/prysk#154, for example), so I don't think the sort is guaranteeing anything.

Also, for future readers, to be clear, Marshal doesn't play a part in taskGraph exectution at all. I'm guessing Nathan is commenting on sorting lists in general (and in this case the Unmarshal part)

chris-olszewski · 2023-01-19T23:24:06Z

cli/internal/run/dry_run.go

+	Dir                    string             `json:"directory"`
+	Dependencies           []string           `json:"dependencies"`
+	Dependents             []string           `json:"dependents"`
+	ResolvedTaskDefinition *fs.TaskDefinition `json:"resolvedTaskDefinition"`


IMO I think the name pretty clear

…in-dry-run

not sure why they changed though 🤔

…in-dry-run

nathanhammond · 2023-01-20T05:50:39Z

cli/internal/fs/turbo_json.go

@@ -301,6 +300,54 @@ func (c *TaskDefinition) UnmarshalJSON(data []byte) error {
 	return nil
 }

+// MarshalJSON deserializes JSON into a TaskDefinition
+func (c *TaskDefinition) MarshalJSON() ([]byte, error) {
+	// Initialize with empty arrays, so we get empty arrays serialized into JSON


For elegance I kinda want to omit empty arrays when printing, but that may make it harder for people to use --dry=json. So I'm not actually proposing this.

Yeah, IMO it's actually important to print the empty arrays, so it's never a question about what the whole config is. In other words, missing key can mean "we don't have this config" or "this config has an empty value", and that makes debugging a bit harder.

github-actions bot added the area: turborepo label Jan 19, 2023

mehulkar force-pushed the mehulkar/turbo-560-log-resolved-task-config-in-dry-run branch from bd7d0d7 to a83ca25 Compare January 19, 2023 08:46

mehulkar added 5 commits January 19, 2023 10:58

Print resolvedTaskDefinition in DryRun

8f64413

Add the same for singlePackage dry runs

2bf00d4

Use existing rawTask for marshaling struct into raw JSON

2596d29

Update dry run test for single package

c978b72

Update dry run tests to print resolved task definition

e81703b

mehulkar force-pushed the mehulkar/turbo-560-log-resolved-task-config-in-dry-run branch from afbf0d6 to e81703b Compare January 19, 2023 21:19

vercel bot deployed to Preview – turbo-site January 19, 2023 21:19 View deployment

mehulkar commented Jan 19, 2023

View reviewed changes

mehulkar marked this pull request as ready for review January 19, 2023 21:38

mehulkar requested a review from a team as a code owner January 19, 2023 21:38

mehulkar requested review from chris-olszewski and NicholasLYang January 19, 2023 21:38

Merge branch 'main' into mehulkar/turbo-560-log-resolved-task-config-…

a6431c1

…in-dry-run

vercel bot deployed to Preview – turbo-site January 19, 2023 21:40 View deployment

chris-olszewski approved these changes Jan 19, 2023

View reviewed changes

mehulkar added 6 commits January 19, 2023 16:28

improve jq syntax

2fe0c78

improve todo

1253e18

Merge branch 'main' into mehulkar/turbo-560-log-resolved-task-config-…

3dca532

…in-dry-run

Add a test to show that slices are sorted

5374897

make sure slices are sorted when marshaling struct into json

7f8615a

update an unrelated test, because hashes changed

ed8dad6

not sure why they changed though 🤔

mehulkar enabled auto-merge (squash) January 20, 2023 02:33

Merge branch 'main' into mehulkar/turbo-560-log-resolved-task-config-…

35bc934

…in-dry-run

mehulkar merged commit 6e33584 into main Jan 20, 2023

mehulkar deleted the mehulkar/turbo-560-log-resolved-task-config-in-dry-run branch January 20, 2023 02:54

nathanhammond reviewed Jan 20, 2023

View reviewed changes

mehulkar mentioned this pull request Feb 6, 2023

Composable turbo.json #2706

Merged

5 tasks

mehulkar mentioned this pull request Mar 1, 2023

Combine TaskSummary with RunSpec for 🪄 #3431

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print resolvedTaskDefinition in DryRun #3377

Print resolvedTaskDefinition in DryRun #3377

mehulkar commented Jan 19, 2023 •

edited

Loading

vercel bot commented Jan 19, 2023 •

edited

Loading

github-actions bot commented Jan 19, 2023 •

edited

Loading

mehulkar Jan 19, 2023

mehulkar Jan 19, 2023

chris-olszewski Jan 19, 2023

mehulkar Jan 20, 2023

nathanhammond Jan 20, 2023

mehulkar Jan 19, 2023

chris-olszewski Jan 19, 2023

mehulkar Jan 19, 2023

chris-olszewski Jan 19, 2023

chris-olszewski Jan 19, 2023

mehulkar Jan 20, 2023

mehulkar Jan 20, 2023

nathanhammond Jan 20, 2023 •

edited

Loading

mehulkar Jan 20, 2023

chris-olszewski Jan 19, 2023

nathanhammond Jan 20, 2023

mehulkar Jan 20, 2023

Print resolvedTaskDefinition in DryRun #3377

Print resolvedTaskDefinition in DryRun #3377

Conversation

mehulkar commented Jan 19, 2023 • edited Loading

vercel bot commented Jan 19, 2023 • edited Loading

github-actions bot commented Jan 19, 2023 • edited Loading

🟢 CI successful 🟢

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

nathanhammond Jan 20, 2023 • 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

mehulkar commented Jan 19, 2023 •

edited

Loading

vercel bot commented Jan 19, 2023 •

edited

Loading

github-actions bot commented Jan 19, 2023 •

edited

Loading

nathanhammond Jan 20, 2023 •

edited

Loading