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.

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

Remove rebuild argument at the CLI level #182

Closed
Tracked by #168
jeremyfowers opened this issue Jul 3, 2024 · 2 comments
Closed
Tracked by #168

Remove rebuild argument at the CLI level #182

jeremyfowers opened this issue Jul 3, 2024 · 2 comments
Labels
cleanup Cleaning up old/unused code and tech debt question Further information is requested

Comments

@jeremyfowers
Copy link
Collaborator

jeremyfowers commented Jul 3, 2024

@danielholanda please consider the following:

Back in the olden days, the rebuild argument was there to help manage the cache for users of the build API. We now have no one using the build API, so that has stopped being relevant.

In the TKML 1.0 era, the rebuild argument was primarily useful for "build once, benchmark elsewhere" workflows. However, we now have the turnkey cache benchmark command which is a much better way to benchmark a prebuilt model. rebuild has also stopped being relevant there.

In the TKML Refresh, commands are much more explicit. If I call turnkey -i model.py discover export-pytorch I expect it to do the thing! Not say "there was a cache hit, so I ignored your command." I find myself always using rebuild=always when I use TKML 2.0.

Finally, in TKML 2.0, it is easy to add a new front-end Stage along the lines of load-build, that would load a build from the cache and pass it to the next Stage. This would enable things like turnkey -i cache/* load-build benchmark to be a much more general replacement for turnkey cache benchmark

@danielholanda questions for you:

  1. Can we get rid of the rebuild argument at the CLI level? My proposal is to set rebuild=always as the default at the CLI->build_api interface. We can keep rebuild=if_needed as the default in the build API.
  2. Should we implement load-build and use it to phase out turnkey cache benchmark?
@jeremyfowers jeremyfowers mentioned this issue Jul 3, 2024
21 tasks
@jeremyfowers jeremyfowers changed the title rebuild="if_needed" is a poor default now, because a cache hit on the CLI will rarely do what the user asked for. Suggest changing the default to "always" or even getting rid of the --rebuild argument at the CLI level. rebuild="if_needed" is a poor default now, because a cache hit on the CLI will rarely do what the user asked for Jul 3, 2024
@jeremyfowers jeremyfowers added question Further information is requested cleanup Cleaning up old/unused code and tech debt labels Jul 3, 2024
@jeremyfowers jeremyfowers added this to the TurnkeyML 2024 Refresh milestone Jul 3, 2024
@jeremyfowers
Copy link
Collaborator Author

cc @danielholanda

@jeremyfowers jeremyfowers changed the title rebuild="if_needed" is a poor default now, because a cache hit on the CLI will rarely do what the user asked for Remove rebuild argument at the CLI level Jul 3, 2024
@danielholanda
Copy link
Collaborator

yes, and yes. It is quite a change but load-build makes it worth it.

This was referenced Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleaning up old/unused code and tech debt question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants