Deprecation warning to assist with migration to new binary names #8283
Conversation
…st to help people notice the binary name change from ggerganov#7809 and migrate to the new filenames.
|
I'm not opposing this change, but can we just print out error from makefile, instead of compiling a new binary? $ make main
The binary 'main' is deprecated.
Please run 'make llama-cli' instead.
make: *** [Makefile:1477: main] Error 1 |
|
That won't solve when users do just make right? Also this will replace their old binary, many were building and accidentally using the main that stuck around rather than the new correct binary, I think this is a more reasonable solution. It doesn't have to be permanent, just for a month or so to reduce the issue clutter |
If I'm understanding @ngxson 's suggestion, we would add this That said, we could also add a filename check to this, and only output this warning if the file exists. I agree with @bartowski1182 -- this is a good thing, but it addresses a slightly different case. I think we could combine the two approaches, and it might be best to do the filename check for this. I.E., only build the replacement binary (and output the warning) if the old one exists -- otherwise, don't build anything new, and don't send any messages. |
|
IMO to address the issue of someone forget to clean up old binaries, we can just remove all old binaries if someone run "make". If they run "make main", then we show the error in my last comment, so eventually no need to compile a new target. |
|
Maybe only if it's successful, don't want make to fail and leave users with no way to run anything |
…or their existence every time so that they are not ignored.
|
Okay, new update to integrate some suggestions, and we now build the legacy replacement binaries more intelligently. Now, the deprecation-warning binaries are ONLY built if a previous version of If one is building from a fresh / updated build, then nothing new is printed or built. Only However, if one is building with legacy binaries present, then for each legacy binary, it will output a message like the following: And the old Because of I'm pretty happy with this latest version, because it is nicely future-proof, and doesn't clutter up the build folder with a bunch of legacy binaries if they're not needed. Now, there's no rush to remove this fix later, and it feels much cleaner to me. How do people feel about this one? |
|
Just realized that a downside to the latest version is that -- if one is building with How best to handle that? I made this change to try and avoid cluttering the build log and build folder with unnecessary messages / binaries if they're not needed. |
|
I think that's a great implementation! It may be worth cluttering the log for now by including the main target and having it error stating that it's deprecated, it can be removed after a few updates to give people time to transition, though probably less relevant now since this has been in the wild for a month or so already |
|
Thanks for the feedback!
I tried experimenting with a few always-logging options (to try and handle the To reply to my earlier concern:
At the end of the day, if someone is typing I propose that we move forward with the PR as-is -- I think it's ready for review. |
…rganov#8283) * Adding a simple program to provide a deprecation warning that can exist to help people notice the binary name change from ggerganov#7809 and migrate to the new filenames. * Build legacy replacement binaries only if they already exist. Check for their existence every time so that they are not ignored.
…rganov#8283) * Adding a simple program to provide a deprecation warning that can exist to help people notice the binary name change from ggerganov#7809 and migrate to the new filenames. * Build legacy replacement binaries only if they already exist. Check for their existence every time so that they are not ignored.
|
Possibly connected to this: #8776 |
As discussed previously, there seems to be a fair bit of confusion still arising from the name change made in #7809. This PR attempts to mitigate some of that confusion by applying a (temporary) fix to help encourage users to migrate to the new filenames.
First suggested here, this PR is merely a dirt-simple program to provide a deprecation warning to point people to the new filenames.
I have mixed feelings about cluttering up the build space with legacy binary filenames, so I only included replacement binaries for the things that I feel are most likely to be included in a pipeline somewhere and the upgrade go un-noticed.
Suggestions for better ways to handle this, or which binaries should be included / excluded are very welcome.
Thank you!
Usage
Previously, continuing to updated source and
makewould leave the old binaries in place, and a common pitfall is for users to continue running old binaries without realizing that names have changed.Now, this is the behavior that one sees:
or:
Guidelines