[wanda] Normalize file mode in digest to 0644/0755 based on executable bit#468
Conversation
|
Reviews in this chain: |
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical inconsistency in how Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
b75ca48 to
c346550
Compare
ed02102 to
4e874c2
Compare
There was a problem hiding this comment.
Code Review
This pull request aims to align the cache key generation between wanda build and wanda digest by pre-computing dependency digests. The changes look good overall, but I've found a couple of issues. There's a potential panic in the Digest command when no NamePrefix is configured, which I've marked as critical. Additionally, there's a bug in how dependency digests are looked up when image tags are present. I've provided suggestions for both issues.
a0e7974 to
362b6ca
Compare
362b6ca to
348a63f
Compare
4e874c2 to
690b275
Compare
348a63f to
f96f966
Compare
f96f966 to
b82d2fc
Compare
374fcd9 to
0f318da
Compare
b82d2fc to
39731ae
Compare
0f318da to
6eb1cc5
Compare
39731ae to
e3cea2e
Compare
6eb1cc5 to
92a7f78
Compare
e3cea2e to
830f3d4
Compare
92a7f78 to
0331d93
Compare
830f3d4 to
d2ddbb8
Compare
aslonnie
left a comment
There was a problem hiding this comment.
it is by design that it is using the output result's digest rather than the input's digest. this is so that when A depends on B, even when B's input changed, as long as B's output has not changed, then the building of A can still be a cache hit.
I think this does raises a new issue, which is how to do pure input based caching.. like for ray core build cache..
we can either:
- save both indexes, one is for the action, one is for the entire dep graph
- take a shortcut and limit digests to only single step actions.. and we can also pin the base image (manylinux base) with image digest.
|
we can maybe talk more in person tomorrow. |
d2ddbb8 to
75d66ba
Compare
| return &tarFileRecord{ | ||
| Name: t.name, | ||
| Mode: meta.Mode, | ||
| Mode: mode, |
There was a problem hiding this comment.
hmm... when in the tarball file's record, can we still use 644 and 755 ?
otherwise this 0 and 1 mode bits is a bit weird.
essentially:
// we only check if a file has exec bit or not
// as a file in git only saves exec bit; other mode bits are determined
// by user's umask set up and might differ from system to system
mode := 0o600 // or should this be 0x644 ?
if (stat.Mode() & 0o100) != 0 {
mode = 0o700 // or should this be 0x755 ?
}
I still feel that what actually matters more is the control of uid and gid.
There was a problem hiding this comment.
I still feel that what actually matters more is the control of uid and gid.
This is to resolve the issue of CI's digest not being reproducible locally. I.e., on the same commit and ensuring both rayci and epoch are set correctly, it was impossible to get a matching digest across CI and locally without manually updating permissions of files in the repo
…e bit Git only tracks two file modes (100644 and 100755), but the digest previously included the full mode from os.Lstat(), which varies with the local umask (e.g. 0644 under umask 0022 vs 0664 under 0002). This caused digest mismatches across environments for identical content. Now record() normalizes to 0644 (non-executable) or 0755 (executable) based solely on the owner-execute bit, matching git's two-state model. Topic: raymake-digest Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: andrew <andrew@anyscale.com>
75d66ba to
1989bb7
Compare
Git only tracks two file modes (100644 and 100755), but the digest previously included the full mode from os.Lstat(), which varies with the local umask (e.g. 0644 under umask 0022 vs 0664 under 0002). This caused digest mismatches across environments for identical content.
Now record() normalizes to 0644 (non-executable) or 0755 (executable) based solely on the owner-execute bit, matching git's two-state model.
Topic: raymake-digest
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Signed-off-by: andrew andrew@anyscale.com