Skip to content

feat: implement stable hash for ProjectModelV1 to improve cache consistency#4094

Merged
Hofer-Julian merged 8 commits intoprefix-dev:mainfrom
baszalmstra:main
Jul 8, 2025
Merged

feat: implement stable hash for ProjectModelV1 to improve cache consistency#4094
Hofer-Julian merged 8 commits intoprefix-dev:mainfrom
baszalmstra:main

Conversation

@baszalmstra
Copy link
Contributor

Summary

Implements custom Hash traits for ProjectModelV1 and all nested types that skip default/empty values in hash calculation for better stability. This allows adding new optional fields without breaking existing hashes, improving cache consistency and reducing unnecessary rebuilds.

Technical Details

  • Removed #[derive(Hash)] from all ProjectModelV1 types
  • Implemented manual Hash traits that ignore:
    • None values in Option fields
    • Empty OrderMap collections
    • Default struct values
  • Used discriminant values for enum variants to ensure consistency
  • Added .gitignore pattern for *.local.* files

Before/After Behavior

Before: Adding any new field would change the hash, breaking cache invalidation

// Adding a new optional field would change hash even if None
pub struct ProjectModelV1 {
    pub name: String,
    pub new_field: Option<String>, // This would break existing hashes
    // ...
}

After: Adding new optional fields with default values doesn't affect hash

// Hash remains stable when new fields are None or empty
impl Hash for ProjectModelV1 {
    fn hash<H: Hasher>(&self, state: &mut H) {
        self.name.hash(state);
        if let Some(ref new_field) = self.new_field {
            new_field.hash(state); // Only hashes if Some()
        }
    }
}

Testing

Added 4 comprehensive tests covering:

  • Hash stability with default values - ensures empty/None values don't change hash
  • Hash changes with meaningful values - verifies real changes still affect hash
  • Binary package spec stability - tests default vs explicit None equality
  • Enum variant consistency - validates same variants hash identically

Benefits

  • Forward compatibility: Adding new optional fields won't break existing hashes
  • Cache stability: Reduces unnecessary cache invalidation and rebuilds
  • Development workflow: Safer to extend ProjectModelV1 structure over time

Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

One comment, then it's ready to go 🚀

Comment on lines +389 to +402
impl Hash for TargetSelectorV1 {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
match self {
TargetSelectorV1::Unix => 0u8.hash(state),
TargetSelectorV1::Linux => 1u8.hash(state),
TargetSelectorV1::Win => 2u8.hash(state),
TargetSelectorV1::MacOs => 3u8.hash(state),
TargetSelectorV1::Platform(p) => {
4u8.hash(state);
p.hash(state);
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add doc comments to all hash functions explaining that the goal is to keep the hash as stable as possible

@baszalmstra
Copy link
Contributor Author

Thanks for the feedback! I've added comprehensive doc comments to all Hash implementations explaining the stability goals:

  • For structs: Documents how we skip None values and empty collections to maintain stability when adding optional fields
  • For enums: Explains the use of discriminant values to keep hashes stable when adding new variants

Each implementation now has clear documentation about why we use custom Hash traits and how they contribute to hash stability. Ready for another look! 🚀

@baszalmstra
Copy link
Contributor Author

baszalmstra commented Jul 7, 2025

I dont think we have to mark this as breaking. This will only break peoples workflow when they use locked source dependencies (which are still in preview).

But would be good to add a note to the release notes.

@baszalmstra
Copy link
Contributor Author

@Hofer-Julian I added a PR to the test-suite to update the hashes there: prefix-dev/pixi-build-testsuite#21

In which order do you want to merge things?

Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Great work!

@Hofer-Julian Hofer-Julian merged commit d7e9714 into prefix-dev:main Jul 8, 2025
71 of 74 checks passed
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.

2 participants