fix: consistent colors for packages #9008
Conversation
turbo-orchestrator
bot
added
needs: triage
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 6 Skipped Deployments
|
|
@ShaharAdskAcc is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thank you for the PR! Do not worry about the integration tests, those will run on CI.
I think what we want is to keep the existing colors, but to hand them out in a consistent way while also making use of all the available colors i.e. not assigning 2 tasks the same color if there's still colors without any tasks.
To be frank, the current setup is overly complicated as it happens during the graph walk which is non-deterministic. I would suggest:
- Prepopulating the color cache with all keys when we start the walk. This will look something like:
for task in engine.tasks().sorted() {
self.color_cache.color_for_key(&task.to_string());
}
- Then also switch so we select colors with the task id as well here
| let colors: [Style; u8::MAX as usize] = | ||
| core::array::from_fn(|index| Style::new().color256(index as u8)); | ||
|
|
||
| colors |
There was a problem hiding this comment.
I don't believe we want to use all of the available colors as:
- many of them aren't especially nice to look at
- many of the colors are very similar e.g. in my tests I got 4 very similar shades of red
There was a problem hiding this comment.
I'm still opposed to using all of the colors here for the reasons listed above.
We can expand our list in a separate PR, but we need to make sure the colors selected are easy to read in most terminals.
|
Got it @chris-olszewski. I see that you suggested using the sorted task name to choose a color and caching it in a hashmap to ensure consistent color assignment. A straightforward approach would be to use an index and increment it each time we need a color for a new task. Since the tasks are ordered by name, this method will generally guarantee the same color for a monorepo that doesn't change much. However, if the order changes—like when adding or removing a package—there’s a good chance the colors will no longer match. We could take a more complex approach to ensure consistent colors even when new packages are added. This could involve hashing the task name and using the result modulo the number of unused colors. (hash % unused_colors.len()) The number of available colors decreases each time a color is assigned. Once all colors are used, we start again with the full color collection. I think that for new projects that add lots of packages and change frequently, this feature might not be very beneficial. While approach 2 is a bit more complicated, it's still straightforward enough. I’m inclined to go with this approach—let me know your thoughts. -Edit- That is what I ended up using it. |
|
Hey @chris-olszewski, I've updated the PR to include the following:
Regarding the issue of similar colors: What do you suggest? We could limit the palette to more distinct colors, but this might result in more frequent repetition. |
There was a problem hiding this comment.
Just the visitor.rs changes in this PR are a huge improvement:
- color order is no longer tied to task execution order
- color is no longer tied to task hashes which change frequently
I would prefer to merge these improvements as they will make life better for many developers. As you mentioned there are still issues with tasks being added/removed from the graph, but that can be addressed in future PRs.
If you would move the changes to color_selector.rs to a new PR, then I would happily merge the remaining changes.
| let colors: [Style; u8::MAX as usize] = | ||
| core::array::from_fn(|index| Style::new().color256(index as u8)); | ||
|
|
||
| colors |
There was a problem hiding this comment.
I'm still opposed to using all of the colors here for the reasons listed above.
We can expand our list in a separate PR, but we need to make sure the colors selected are easy to read in most terminals.
| hasher.finish() | ||
| } | ||
|
|
||
| fn get_color_id_by_key(&mut self, key: &str) -> usize { |
There was a problem hiding this comment.
This implementation suffers from the same issue as the existing one where adding/removing tasks will result in a changed color for the task when there are hash collisions. If both a#build and b#build get the same initial color_id, then removing a#build will result in b#build changing colors.
Given we're targeting such a small hash (5 at the moment, 256 if we allow for all colors) hash collisions are fairly likely.
|
Sure @chris-olszewski, |
|
@chris-olszewski I excluded black because most users use it as their background color. This change should reduce color collisions between packages, and having 14 colors, as opposed to 5, should make it easier to visually distinguish issues. |
|
Hey @chris-olszewski, is increasing the number of colors worth pursuing, or do you prefer to skip it? I'm unsure whether to continue testing different colors due to the lack of response. |
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
1 similar comment
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
PR Description
This PR addresses issue #2564 tagged as a good first issue.
Changes:
Additional Notes:
turborepo-tests. It might be beneficial to provide a more detailed explanation of the testing process in theCONTRIBUTING.mdfile to assist future contributors.Testing Instructions