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

[RFC] Proposal for tune cat Command #2281

Open
Ankur-singh opened this issue Jan 19, 2025 · 2 comments
Open

[RFC] Proposal for tune cat Command #2281

Ankur-singh opened this issue Jan 19, 2025 · 2 comments
Assignees
Labels
discussion Start a discussion rfc Request for comments

Comments

@Ankur-singh
Copy link
Contributor

First of all, thank you very much for the wonderful package. I’ve started actively looking at the source code, and I must say it’s an absolute pleasure to read. It was difficult to stop myself from contributing.

Context:

Currently, the tune ls command allows users to list all available recipes and their corresponding configurations. However, it doesn’t provide an easy way to view the actual content of a configuration file itself. Before modifying or overriding parameters, users often need to inspect the contents of a specific configuration to understand its structure and settings.

Proposal:

A new command to the torchtune framework: tune cat.

The tune cat command would fill this gap by allowing users to view the contents of a specific configuration file. It would work similarly to kubectl describe, providing a clear and detailed view of the configuration content.

The ideal workflow could be as follows:

  1. Use tune ls to list all available configurations.
  2. Use tune cat <config> to view the contents of a specific config file.
  3. After inspecting the config, users can:
    • Override existing parameters directly via the command line, or
    • Copy the config using tune cp and modify it locally.

This feature would make the workflow more intuitive and user-friendly.

I’d love to hear your thoughts on this proposal. Do you think tune cat would be a valuable addition to the framework? Are there any other suggestions or improvements you would recommend?

Thank you for your time and feedback!

@RdoubleA RdoubleA changed the title Proposal for tune cat Command [RFC] Proposal for tune cat Command Jan 19, 2025
@RdoubleA RdoubleA added rfc Request for comments discussion Start a discussion labels Jan 19, 2025
@joecummings
Copy link
Contributor

@Ankur-singh Thanks for your kind words and even kinder contributions to the torchtune library! They are much appreciated :)

On the whole, I'm very supportive of tune cat. Couple of (minor) follow ups:

  1. Do you imagine extending this functionality to work with recipes as well? The tune ls and tune cp commands cover both recipes and configs, so tune cat only working with configs would break that paradigm a little bit. However, the value of cat for configs is clear whereas the value of cat for recipes is not. Personally, I don't see this extending to recipes, but that means that you should provide a clear error message if someone wants to cat a recipe file, which also means you'll need to know what is a recipe, a config, and is not in the torchtune system.
  2. Should a user be able to tune cat a local file?

I think this RFC is clear enough that you can go ahead and get started on this work and any other comments can be resolved on the PR. I'd recommend familiarizing yourself with the CLI code and reading through the CLI RFC for more information on our thoughts on CLIs in general.

Aside: As you develop, you'll see that our CLI is incredibly slowwwww. This is likely due to poor management of imports and is something we'd very much like to fix. Definitely don't combine it with this feature, but if you think of elegant solutions as you work deeply with the CLI code, please don't hesitate to share!

@Ankur-singh
Copy link
Contributor Author

  1. I'm with you on the first point. tune cat for recipes is not very obvious. Not supporting recipes would make the cat command digress from ls and cp but having clear error message and docs should help users with the correct usage.
  2. Ideally yes, this will help keep the behavior consistent with other commands.

Can you please assign it to me? Thanks for the resources :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Start a discussion rfc Request for comments
Projects
None yet
Development

No branches or pull requests

3 participants