Refactor: Rename BlobReader to BlobFinder #818
Conversation
Renamed BlobReader and BlobWriter to BlobFinder as suggested by @janwas in TODO. Also Updated associated files, tests, and CMakeLists.txt.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hello and happy holidays :) Thanks for tackling this. Would you mind retargeting this toward the dev branch? We are unable to merge into main directly. |
|
Hi Jan, I've retargeted this to the dev branch, signed the CLA, and merged the latest dev changes to resolve the conflicts. Everything should be ready for review now. I’m planning to focus heavily on low-level optimization and ML infrastructure this year, so I'll be keeping an eye on the issue tracker for more ways to contribute to gemma.cpp. Thanks! |
|
Thanks! CLA looks good. Nice, interesting topic! It's a fast moving field, lots going on :) |
|
Thanks for the catch! I really should have spotted those broken includes before pushing. My bad. I've reverted the file names to blob_store and fixed the include in model_store.h, while keeping the BlobReader -> BlobFinder class rename. It should be good to go now! And yeah, happy to be contributing to the 'lots' going on in the field! Hehe. Looking forward to more |
io/blob_store.h
Outdated
| #ifndef THIRD_PARTY_GEMMA_CPP_IO_BLOB_STORE_H_ | ||
| #define THIRD_PARTY_GEMMA_CPP_IO_BLOB_STORE_H_ | ||
| #ifndef THIRD_PARTY_GEMMA_CPP_IO_BLOB_FINDER_H_ | ||
| #define THIRD_PARTY_GEMMA_CPP_IO_BLOB_FINDER_H_ |
There was a problem hiding this comment.
Given this file is called blob_store.h, and this is an include guard for the whole file (which has more than just BlobFinder in it), this name shouldn't be changed.
io/blob_store.h
Outdated
|
|
||
| // Reads `BlobStore` header, converts keys to strings and creates a hash map for | ||
| // faster lookups. | ||
| // TODO(janwas): rename to BlobFinder or similar. |
There was a problem hiding this comment.
This todo should probably be removed?
| // TODO(janwas): rename to BlobFinder or similar. |
io/blob_store_test.cc
Outdated
|
|
||
| #if !HWY_TEST_STANDALONE | ||
| class BlobStoreTest : public testing::Test {}; | ||
| class BlobFinderTest : public testing::Test {}; |
There was a problem hiding this comment.
I think this name shouldn't change, since these tests also test BlobWriter.
|
I was aiming for strict alignment between the new BlobFinder class and its metadata, but since the header is a shared container for BlobWriter and the other store utilities, you're right, and I've reverted to the file-based identifiers. it should all be fixed now. Regarding the 'fatal' build errors Jan mentioned in the model store, I'll fix those in a separate PR immediately after this. wanted to keep this focused. |
|
Let's get this merged in so OIa can be officially recognized as a Google employee! 😄 PS. this is pure gold 😂
|
| class BlobStoreTest : public testing::Test {}; | ||
| #endif | ||
|
|
||
| // Change the first argument from BlobFinderTest to BlobStoreTest |
There was a problem hiding this comment.
This guy used AI just to rename ONE (1) class. Honestly if you guys merge this in you are just invitng more trouble in future from people following this guy's approach just so they can put "core contibutor @google/deepmind" in their bio. Also how is he raising PR's here without even having basic understanding of bazel.
There was a problem hiding this comment.
Yes, but it will be setting a precedent that lazy, AI PRs built on zero understanding of the actual code will be merged, for what? To ruin the code slowly PR by PR creating bigger problems in the future just so one guy can say "I work at Google deep mind"? (PR OPs actual statement)
There was a problem hiding this comment.
I don't know why he would bloody lie unprovoked lol 😂
|
https://x.com/i/status/2010354903428833692 The link to the post where he said he works for Google just after writing an AI-unmerged PR that addresses a TODO, @olaotesile an unmerged PR doesn't make you a contributor or worker you need to do better😂 |
|
Nice one. Hoping to contribute myself. |
|
Congrats, you now officially work at Google in the same way I work at Linux because I ran apt update. |
|
Can some help him merge the PR please 🙏🏾😭😭 Man needs his profile to be a living proof
|
|
bro deleted the tweet. he brought this L on himself |
|
Aged like milk |
|
the OG tweet |
|
Icarus flew too close to the Sun of Google Inc💔 |
|
Never enjoyed PR comments like this until today 😂😂 thank you Ola |
|
Unfortunately "It should be good to go now!" and "it should all be fixed now." was inaccurate, the code still fails to build. |
This comment was marked as off-topic.
This comment was marked as off-topic.
17 participants
Renamed BlobReader and BlobWriter to BlobFinder as suggested by @janwas in TODO. Also Updated associated files, tests, and CMakeLists.txt.
Addresses the TODO in io/blob_store.h by renaming the classes and files to BlobFinder/BlobWriter for better clarity. Verified file renames and updated CMake configuration.