refactor(turborepo): Package Detection #7549

NicholasLYang · 2024-02-28T21:59:18Z

Description

Factored out package detection, i.e. the part where we take a file and map it to the package into a trait. This allows us to have multiple implementations, such as a default one that only uses the package graph, and a more advanced implementation that uses the package graph and turbo.json.

Testing Instructions

Closes TURBO-2465

github-actions · 2024-02-28T22:00:08Z

🟢 Turbopack Benchmark CI successful 🟢

Thanks

vercel · 2024-02-28T22:00:30Z

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

Name	Status	Preview	Comments	Updated (UTC)
examples-nonmonorepo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 5, 2024 10:34pm
examples-tailwind-web	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Mar 5, 2024 10:34pm
rust-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 5, 2024 10:34pm

7 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 5, 2024 10:34pm
examples-designsystem-docs	⬜️ Ignored (Inspect)	Visit Preview	Mar 5, 2024 10:34pm
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 5, 2024 10:34pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview	Mar 5, 2024 10:34pm
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 5, 2024 10:34pm
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 5, 2024 10:34pm
examples-vite-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 5, 2024 10:34pm

github-actions · 2024-02-28T22:34:57Z

🟢 CI successful 🟢

Thanks

mehulkar

Am I correct to understand that the new Turbo implementation of PackageDetector isn't being used in this PR? I would prefer to see that implementation when it's used (by watch mode?)
Can you also update the usage in the JS package?

turbo/packages/turbo-repository/rust/src/lib.rs

Line 160 in 306f130

let mapper = ChangeMapper::new(&self.graph, vec![], vec![]);
- CI should be failing on this change for that package, but I'm not sure if it's setup right
  - edit: fixing, CI was not running because of the filter fix(ci) remove filter when running @turbo/repository tests #7551
For Vercel, we will also need an implementation that returns all dependents. Just confirming that another implementation of this trait would be able to deliver that feature? If so... nice!

crates/turborepo-lib/src/package_detector.rs

crates/turborepo-lib/src/run/scope/change_detector.rs

NicholasLYang · 2024-02-29T16:30:47Z

Adding the usage for TurboJsonPackageDetector is a little out of scope cause that would involve pulling in the watch mode code, which would make this a pretty big PR

chris-olszewski

This PR is labeled as a refactor, but has some pretty significant behavior changes for both turbo and @turbo/repository.

I'd feel far more comfortable with the refactors remaining, but actual behavior changes coming in a different PR with more testing to get a better picture of how this could affect our behavior in various scenarios.

crates/turborepo-repository/src/package_graph/builder.rs

crates/turborepo-repository/src/change_mapper.rs

NicholasLYang · 2024-02-29T18:11:31Z

I don't believe there's a behavior change here. With the current code, we return the root changed if there exists any changed file that is not in a package. All I'm doing is removing the redundant code that also checks if package.json or turbo.json are changed when they're by definition not in any package and therefore fall under the first check.

chris-olszewski

I do feel that this makes it harder to reasons about this code, but it seems necessary for what we want to do in the future. Relying on your manual testing to verify there aren't accidental behavior changes anymore.

chris-olszewski · 2024-03-04T20:34:48Z

crates/turborepo-lib/src/global_deps_package_detector.rs

+        let turbo_json = TurboJson::default();
+        let turbo_package_detector = GlobalDepsPackageDetector::new(
+            &pkg_graph,
+            turbo_json.global_deps.iter().map(|s| s.as_str()),


Nit: can we just the value here instead of depending on the default value of TurboJson?

Sorry, I'm confused. In what part are we depending on the default value?

chris-olszewski · 2024-03-04T20:35:50Z

crates/turborepo-lib/src/global_deps_package_detector.rs

+            // PackageDetection::Package(WorkspacePackage::root()).
+            PackageDetection::All => {
+                let cleaned_path = path.clean();
+                let in_global_deps = self.global_deps_matcher.is_match(cleaned_path.as_str());


I'm having trouble tracing this code. Does the global_deps_matcher always get the default global deps of package.json/turbo.json added to it? Otherwise we'll incorrectly map a package.json change just be a root package change instead of a all change.

Or I'm misunderstanding these changes completely and this codepath isn't hit in that scenario.

Edit: I was misunderstanding, default global deps are checked at ChangeMapper which uses a package detector if a default global change isn't detected

chris-olszewski · 2024-03-04T20:47:00Z

crates/turborepo-repository/src/change_mapper.rs

        }
    }

+    fn default_global_file_changed(
+        changed_files: &HashSet<AnchoredSystemPathBuf>,
+    ) -> Result<bool, ChangeMapError> {


Suggested change

) -> Result<bool, ChangeMapError> {

) -> bool {

crates/turborepo-lib/src/global_deps_package_detector.rs

Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>

mehulkar

A few inline comments. Another question I have is:

I want to be able to return a list of the full tree of "affected" packages. How/where should this modeled? I see there are two levels of abstraction (ChangeMapper takes a list of files and PackageDetector takes one file).

Where would you implement the dependency graph walk so that some_method(files) will return all affected packages, and not just the direct mapping from file to package
How would you organize that with the global (vs not) feature?

mehulkar · 2024-03-05T17:46:19Z

crates/turborepo-lib/src/global_deps_package_detector.rs

+                if in_global_deps {
+                    PackageDetection::All
+                } else {
+                    PackageDetection::Package(WorkspacePackage::root())


Can we return None here? This implementation implies that we're talking about globalDependencies in turbo.json. Is there a scenario in which it's valuable to know that the root package changed or would it just get ignored by the caller?

Higher level: I'm concerned about inventing a semantic meaning of WorkspacePackage::root(). So far, "root package changed" has roughly implied that "all packages changed". But in this PR, we're separating the concepts of "all" and "root package", so we can no longer ignore paper over what a "root package" is.

Our definition of package is roughly equivalent to the package managers definition, and there are no package managers that consider the root of a "workspace" a package itself.

What do you think?

So I think there's a few misunderstandings (not helped by the multiple iterations of this PR):

It is helpful to know that the root package changed, because you can have root tasks that can get run, say if you define a //#build entry in pipeline. We can't remove this root package return because that would be a breaking change for people with root package tasks.

Root package changed does not imply all packages changed. I thought it did but @chris-olszewski explained to me that it does not. I think it should in an ideal world (and I think a setting to enable that behavior is worth discussing but out of scope).

Therefore separating between the two concepts is fine because that is the existing behavior.

As for how our model relates to a package manager, I agree that we should stay closer to the package manager's semantics, but unfortunately we can't due to existing behavior. Like we should treat a root package change as a global change because that's what package managers effectively do, but we have to wait for a breaking change to make that possible.

mehulkar · 2024-03-05T17:58:12Z

crates/turborepo-repository/src/change_mapper.rs

+    }
+}
+
+impl<'a> PackageDetector for DefaultPackageDetector<'a> {


nit: I'd love to see this organized something like this:

crates/turborepo-lib/src/package_change_detector/default.rs crates/turborepo-lib/src/package_change_detector/global_deps.rs

We can't do that because that would be a circular dependency between turborepo-lib and turborepo-repository. I totally agree that would be a more sensible organization

mehulkar · 2024-03-05T18:05:34Z

crates/turborepo-repository/src/change_mapper.rs

+pub trait PackageDetector {
+    fn detect_package(&self, file: &AnchoredSystemPath) -> PackageDetection;
+}


nit: I don't love the naming here. In english, this trait/method isn't detecting packages (which sounds more like what turbo info does), it is inferring how a file change affects a known list of packages. Just a suggestion:

Suggested change

pub trait PackageDetector {

fn detect_package(&self, file: &AnchoredSystemPath) -> PackageDetection;

}

pub trait WorkspaceChangeDetector {

fn get_changed_package(&self, file: &AnchoredSystemPath) -> WorkspaceChange;

}

Hmm yeah agreed. I do feel like WorkspaceChangeDetector is a little confusing too. Maybe PackageChangeMapper? It's mapping a single change to a package

yeah I didn't want to use "change mapper" since we use that upstream already 😓 . In lieu of an obvious answer, no need to make changes :)

#7655) ### Description I forgot to do this in #7549, whoops. Renames to PackageChangeMapper according to PR feedback. ### Testing Instructions  Closes TURBO-2559

NicholasLYang requested a review from a team as a code owner February 28, 2024 21:59

NicholasLYang requested review from mehulkar and chris-olszewski February 28, 2024 21:59

turbo-orchestrator bot added created-by: turborepo owned-by: turborepo labels Feb 28, 2024

vercel bot deployed to Preview – rust-docs February 28, 2024 22:06 View deployment

vercel bot deployed to Preview – examples-nonmonorepo February 28, 2024 22:46 View deployment

vercel bot deployed to Preview – rust-docs February 28, 2024 22:55 View deployment

mehulkar reviewed Feb 29, 2024

View reviewed changes

crates/turborepo-lib/src/package_detector.rs Outdated Show resolved Hide resolved

crates/turborepo-lib/src/package_detector.rs Outdated Show resolved Hide resolved

crates/turborepo-lib/src/run/scope/change_detector.rs Outdated Show resolved Hide resolved

mehulkar mentioned this pull request Feb 29, 2024

fix(ci) remove filter when running @turbo/repository tests #7551

Merged

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 16:12 View deployment

turbo-orchestrator bot added the pkg: turbo-repository label Feb 29, 2024

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 16:17 View deployment

vercel bot deployed to Preview – rust-docs February 29, 2024 16:31 View deployment

NicholasLYang requested a review from mehulkar February 29, 2024 16:36

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 17:31 View deployment

chris-olszewski requested changes Feb 29, 2024

View reviewed changes

crates/turborepo-repository/src/package_graph/builder.rs Outdated Show resolved Hide resolved

crates/turborepo-repository/src/change_mapper.rs Outdated Show resolved Hide resolved

crates/turborepo-repository/src/change_mapper.rs Show resolved Hide resolved

vercel bot deployed to Preview – rust-docs February 29, 2024 17:40 View deployment

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 18:06 View deployment

vercel bot deployed to Preview – rust-docs February 29, 2024 18:15 View deployment

NicholasLYang requested a review from chris-olszewski February 29, 2024 18:18

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 18:40 View deployment

vercel bot deployed to Preview – rust-docs February 29, 2024 18:48 View deployment

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 19:02 View deployment

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 19:04 View deployment

NicholasLYang added 4 commits March 4, 2024 11:18

Removing change post-rebase

4233065

Fix call site and comment

c22d2ec

Tidying up

3386cd3

Fixing tests

c3f48bb

NicholasLYang force-pushed the refactor/change-mapper-package-detection branch from facf6cd to c3f48bb Compare March 4, 2024 16:18

vercel bot deployed to Preview – examples-nonmonorepo March 4, 2024 16:19 View deployment

vercel bot deployed to Preview – rust-docs March 4, 2024 16:36 View deployment

Fix test

1b9de4d

vercel bot deployed to Preview – examples-nonmonorepo March 4, 2024 18:22 View deployment

vercel bot deployed to Preview – rust-docs March 4, 2024 18:31 View deployment

chris-olszewski approved these changes Mar 4, 2024

View reviewed changes

Update crates/turborepo-lib/src/global_deps_package_detector.rs

fdbbae6

Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>

vercel bot deployed to Preview – examples-nonmonorepo March 4, 2024 22:33 View deployment

vercel bot deployed to Preview – rust-docs March 4, 2024 22:42 View deployment

PR feedback

b191cb3

vercel bot deployed to Preview – examples-nonmonorepo March 5, 2024 17:32 View deployment

vercel bot deployed to Preview – rust-docs March 5, 2024 17:41 View deployment

mehulkar reviewed Mar 5, 2024

View reviewed changes

mehulkar approved these changes Mar 5, 2024

View reviewed changes

PR feedback

c4ca9e2

vercel bot deployed to Preview – examples-nonmonorepo March 5, 2024 22:15 View deployment

Fix up names

fb4f85b

vercel bot deployed to Preview – examples-nonmonorepo March 5, 2024 22:19 View deployment

vercel bot deployed to Preview – rust-docs March 5, 2024 22:34 View deployment

NicholasLYang enabled auto-merge (squash) March 5, 2024 22:36

NicholasLYang merged commit 95cbbb0 into main Mar 5, 2024
57 checks passed

NicholasLYang deleted the refactor/change-mapper-package-detection branch March 5, 2024 22:56

NicholasLYang mentioned this pull request Mar 6, 2024

refactor(turborepo): Renaming -PackageDetector to -PackageChangeMapper #7655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(turborepo): Package Detection #7549

refactor(turborepo): Package Detection #7549

NicholasLYang commented Feb 28, 2024 •

edited by padmaia

github-actions bot commented Feb 28, 2024 •

edited

vercel bot commented Feb 28, 2024 •

edited

github-actions bot commented Feb 28, 2024 •

edited

mehulkar left a comment •

edited

NicholasLYang commented Feb 29, 2024

chris-olszewski left a comment

NicholasLYang commented Feb 29, 2024

chris-olszewski left a comment

chris-olszewski Mar 4, 2024

NicholasLYang Mar 5, 2024

chris-olszewski Mar 4, 2024

chris-olszewski Mar 4, 2024

mehulkar left a comment

mehulkar Mar 5, 2024

NicholasLYang Mar 5, 2024

mehulkar Mar 5, 2024

NicholasLYang Mar 5, 2024

mehulkar Mar 5, 2024

NicholasLYang Mar 5, 2024

mehulkar Mar 5, 2024

refactor(turborepo): Package Detection #7549

refactor(turborepo): Package Detection #7549

Conversation

NicholasLYang commented Feb 28, 2024 • edited by padmaia

Description

Testing Instructions

github-actions bot commented Feb 28, 2024 • edited

🟢 Turbopack Benchmark CI successful 🟢

vercel bot commented Feb 28, 2024 • edited

github-actions bot commented Feb 28, 2024 • edited

🟢 CI successful 🟢

mehulkar left a comment • edited

Choose a reason for hiding this comment

NicholasLYang commented Feb 29, 2024

chris-olszewski left a comment

Choose a reason for hiding this comment

NicholasLYang commented Feb 29, 2024

chris-olszewski left a 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

mehulkar left a 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

NicholasLYang commented Feb 28, 2024 •

edited by padmaia

github-actions bot commented Feb 28, 2024 •

edited

vercel bot commented Feb 28, 2024 •

edited

github-actions bot commented Feb 28, 2024 •

edited

mehulkar left a comment •

edited