Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(core): ensure external dependency hashes are resolved in a deterministic way #17926

Merged
merged 3 commits into from
Jul 5, 2023
Merged

fix(core): ensure external dependency hashes are resolved in a deterministic way #17926

merged 3 commits into from
Jul 5, 2023

Conversation

skrtheboss
Copy link
Contributor

@skrtheboss skrtheboss commented Jul 3, 2023 •
edited
Loading

Current Behavior

External dependency hashes are not deterministic and could cause cache misses, since the hash is dependent on which task was hashed when.

Expected Behavior

Task hashes should not depend on which task have been hashed earlier.

Related Issue(s)

Fixes #17917

@skrtheboss skrtheboss requested a review from a team as a code owner July 3, 2023 13:01
@skrtheboss skrtheboss requested a review from FrozenPandaz July 3, 2023 13:01
@vercel
Copy link

vercel bot commented Jul 3, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 5, 2023 6:27am

@nx-cloud
Copy link

nx-cloud bot commented Jul 3, 2023 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 9cd8f87. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@skrtheboss skrtheboss force-pushed the fix/task-hasher-not-deterministic branch from 443d9bb to 879e3ed Compare July 3, 2023 14:08
@skrtheboss
Copy link
Contributor Author

The regression has been introduced by #16926.

This issue is preventing us to upgrade to 16.4.x, since our build cache no longer works correctly, and even without changing anything, the build needs to re-execute nearly all targets.

@skrtheboss
Copy link
Contributor Author

Since @meeroslav is the author of the pull request, i am going to mention him.

meeroslav
meeroslav approved these changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@meeroslav meeroslav left a comment

Choose a reason for hiding this comment

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

Looks good to me

@meeroslav
Copy link
Contributor

Have you pinpointed where (and why) exactly is the error happening on CI? The proposed PR looks ok, and I think it improves accuracy, but I don't see any evidence supporting that this would actually fix the linked problem.

@meeroslav meeroslav self-assigned this Jul 4, 2023
@skrtheboss
Copy link
Contributor Author

@meeroslav i have added a test which fails if the patch is not applied.

The hashed value of appB was dependent if appA was hashed prior to it.

@meeroslav meeroslav added type: bug scope: core core nx functionality labels Jul 4, 2023
@skrtheboss skrtheboss force-pushed the fix/task-hasher-not-deterministic branch from 879e3ed to 910d943 Compare July 4, 2023 19:24
@skrtheboss
Copy link
Contributor Author

Rebased and applied requested changes in the fixup commit.

@skrtheboss skrtheboss requested a review from FrozenPandaz July 4, 2023 19:40
meeroslav
meeroslav requested changes Jul 4, 2023
View reviewed changes
packages/nx/src/hasher/task-hasher.ts Outdated Show resolved Hide resolved
packages/nx/src/hasher/task-hasher.ts Outdated Show resolved Hide resolved
packages/nx/src/hasher/task-hasher.ts Outdated Show resolved Hide resolved
packages/nx/src/hasher/task-hasher.ts Outdated Show resolved Hide resolved
skrtheboss added 3 commits July 5, 2023 08:19
@skrtheboss
fix(core): ensure external dependency hashes are resolved in a determ…
7b933ca 
…inistic way

External dependency hashes were not deterministic and could cause cache misses,
since the hash was dependent on which task was hashed when.
This was caused by the externalDepsHashCache which was filled with values which are
dependent on the visited set.

Fixes nrwl#17917
@skrtheboss
fixup! fix(core): ensure external dependency hashes are resolved in a…
3564d41 
… deterministic way
@skrtheboss
fixup! fix(core): ensure external dependency hashes are resolved in a…
9cd8f87 
… deterministic way
@skrtheboss skrtheboss force-pushed the fix/task-hasher-not-deterministic branch from 910d943 to 9cd8f87 Compare July 5, 2023 06:23
@skrtheboss
Copy link
Contributor Author

Hi @meeroslav i have rebased again and applied requested changes in the 2nd fixup commit.

@skrtheboss skrtheboss requested a review from meeroslav July 5, 2023 06:24
meeroslav
meeroslav approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@meeroslav meeroslav left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@meeroslav meeroslav self-requested a review July 5, 2023 09:39
@FrozenPandaz
Copy link
Collaborator

Thank you for making the changes! LGTM 🎉

@FrozenPandaz FrozenPandaz merged commit 65adb94 into nrwl:master Jul 5, 2023
@skrtheboss skrtheboss deleted the fix/task-hasher-not-deterministic branch July 5, 2023 14:53
@skrtheboss
Copy link
Contributor Author

@FrozenPandaz thank you for the feedback and for merging this, do you have any estimate on when this will be released?

@atsjo
Copy link

atsjo commented Jul 5, 2023

Betting this will fix the caching problems I have observed as well since 16.2.x, and reported in #17081
Managed to make it work most of the time by restructuring one of the projects in my repo, but sometimes it still doesn't use the cache, and it seems non-deterministic.

@meeroslav
Copy link
Contributor

@skrtheboss we are planning a patch today

@skrtheboss skrtheboss mentioned this pull request Jul 7, 2023
Merged
FrozenPandaz pushed a commit that referenced this pull request Jul 7, 2023
@skrtheboss @FrozenPandaz
fix(core): ensure external dependency hashes are resolved in a determ…
699af69 
…inistic way (#17926)

(cherry picked from commit 65adb94)
@timonmasberg timonmasberg mentioned this pull request Jul 11, 2023
Merged
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@FrozenPandaz FrozenPandaz FrozenPandaz approved these changes

@meeroslav meeroslav meeroslav approved these changes

Assignees

@meeroslav meeroslav

Labels
scope: core core nx functionality type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cache Invalidation Issue in CI/CD Pipeline (Jenkins) - Inconsistent Hash Value
4 participants
@skrtheboss @meeroslav @FrozenPandaz @atsjo