-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: port task graph validation #5703
Changes from 2 commits
a4a52cd
3d0185f
eb8e27d
db8d23b
3240589
e03e561
ceae70f
35135df
525e1ee
b350321
dedea80
03578db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,11 @@ use std::{ | |
pub use builder::EngineBuilder; | ||
use petgraph::Graph; | ||
|
||
use crate::{run::task_id::TaskId, task_graph::TaskDefinition}; | ||
use crate::{ | ||
package_graph::{PackageGraph, WorkspaceName}, | ||
run::task_id::TaskId, | ||
task_graph::TaskDefinition, | ||
}; | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] | ||
pub enum TaskNode { | ||
|
@@ -110,4 +114,110 @@ impl Engine<Built> { | |
.collect(), | ||
) | ||
} | ||
|
||
pub fn validate( | ||
&self, | ||
package_graph: &PackageGraph, | ||
concurrency: u32, | ||
) -> Result<(), Vec<ValidateError>> { | ||
// TODO(olszewski) once this is hooked up to a real run, we should | ||
// see if using rayon to parallelize would provide a speedup | ||
let (persistent_count, mut validation_errors) = self | ||
.task_graph | ||
.node_indices() | ||
.map(|node_index| { | ||
let TaskNode::Task(task_id) = self | ||
.task_graph | ||
.node_weight(node_index) | ||
.expect("graph should contain weight for node index") | ||
else { | ||
// No need to check the root node if that's where we are. | ||
return Ok(false); | ||
}; | ||
let is_persistent = self | ||
.task_definitions | ||
.get(task_id) | ||
.map_or(false, |task_def| task_def.persistent); | ||
|
||
for dep_index in self | ||
.task_graph | ||
.neighbors_directed(node_index, petgraph::Direction::Outgoing) | ||
{ | ||
let TaskNode::Task(dep_id) = self | ||
.task_graph | ||
.node_weight(dep_index) | ||
.expect("node index not in graph") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: a clearer explanation like 'dep_index comes from iterating the graph and must be there' Seems like a very common thing to need, I'm surprised they don't have an access pattern that simplifies this... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that it's surprising that there isn't an "unwrap-less" method for getting neighbor weights. There's |
||
else { | ||
panic!("{task_id} depends on root task node"); | ||
}; | ||
|
||
let task_definition = self.task_definitions.get(dep_id).ok_or_else(|| { | ||
ValidateError::MissingTask { | ||
task_id: dep_id.to_string(), | ||
package_name: dep_id.package().to_string(), | ||
} | ||
})?; | ||
|
||
let package_json = package_graph | ||
.package_json(&WorkspaceName::from(dep_id.package())) | ||
.ok_or_else(|| ValidateError::MissingPackageJson { | ||
package: dep_id.package().to_string(), | ||
})?; | ||
if task_definition.persistent | ||
&& package_json.scripts.contains_key(dep_id.task()) | ||
{ | ||
return Err(ValidateError::DependencyOnPersistentTask { | ||
persistent_task: dep_id.to_string(), | ||
dependant: task_id.to_string(), | ||
}); | ||
} | ||
} | ||
|
||
Ok(is_persistent) | ||
}) | ||
.fold((0, Vec::new()), |(mut count, mut errs), result| { | ||
match result { | ||
Ok(true) => count += 1, | ||
Ok(false) => (), | ||
Err(e) => errs.push(e), | ||
} | ||
(count, errs) | ||
}); | ||
|
||
if persistent_count > concurrency { | ||
validation_errors.push(ValidateError::PersistentTasksExceedConcurrency { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this will address the current issue where persistent tasks count towards the concurrency limit despite not being targeted by filter / scoping rules? (sorry if this is obvious, I am not sure if this is validated before or after we calculate the subgraph) To elaborate, if I have 100 packages with a persistent task in them and then run with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's interesting, both in Go/Rust the task graph is constructed using only packages that are in scope. Didn't realize this was an issue. Either way this is meant to be the port of the Go behavior. |
||
persistent_count, | ||
concurrency, | ||
}) | ||
} | ||
|
||
match validation_errors.is_empty() { | ||
true => Err(validation_errors), | ||
false => Ok(()), | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug, thiserror::Error)] | ||
pub enum ValidateError { | ||
#[error("Cannot find task definition for {task_id} in package {package_name}")] | ||
MissingTask { | ||
task_id: String, | ||
package_name: String, | ||
}, | ||
#[error("Cannot find package {package}")] | ||
MissingPackageJson { package: String }, | ||
#[error("\"{persistent_task}\" is a persistent task, \"{dependant}\" cannot depend on it")] | ||
DependencyOnPersistentTask { | ||
persistent_task: String, | ||
dependant: String, | ||
}, | ||
#[error( | ||
"You have {persistent_count} persistent tasks, but `turbo` is configured for concurrency \ | ||
of {concurrency}. Set --concurrency to at least {persistent_count}" | ||
)] | ||
PersistentTasksExceedConcurrency { | ||
persistent_count: u32, | ||
concurrency: u32, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't even know you could apply this syntax on enums like that. Neat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too learned that this was possible from @NicholasLYang and I've been in love ever since.