[Discussion] "tune download" behavior is unintuitive

[kartikayk]

As I walked through tune download as a user, I found the behavior unintuitive. Concretely:

• Name. download should be renamed to hf_download. That's what this command does and without any information, I get the impression that I can use this to download a model from any source.

• Restrictions. download currently restricts my choices to what is defined in _SUPPORTED_MODEL_REPO_IDS. This seems arbitrarily restrictive since I could download a model, write my own conversion script and use TorchTune to finetune the model. This was the use case I had in mind and i ended up having to modify code to do this. The behavior I would expect in this case is, let the user download the model they want but raise a warning if the model is not supported OOTB. Anyways convert-checkpoint will raise an error, so I'm not too concerned about the user shooting themselves in the foot by fine-tuning a model we don't support.

[joecummings]

Thanks for opening this, @kartikayk!

A couple of thoughts:

1. Name: The command takes a slight UX hit by renaming to hf_download. CLI commands should be one word (see my CLI RFC for literature on this notion). convert_checkpoint is a special case where convert isn't enough to describe the command sufficiently. In addition, the download command's help description is:
   and the "repo-id" name also mentions the HuggingFace Hub making it pretty clear where the downloads are coming from.

2. This makes sense to me! There's some significant thought in how we would plan to support the addition of these conversion scripts. Also, I want @msaroufim 's thoughts b/c he suggested having download convert a checkpoint by default (via a flag), so maybe we would throw an explicit error in that case and then suggest that people turn off the flag to download an arbitrary model from the hub?

[kartikayk]

Fair point on the name!

Hmm, I don't think download and convert-checkpoint should be part of the same flow. It's an extra step but the separation makes sense to me, especially since we cannot and will not have checkpoint conversion scripts ready for every model. This goes into the extensibility side of TorchTune. Let users download the model they want (but warn them) and let them build (and contribute) the checkpoint conversion scripts. I'd be in favor of the warning approach here.

[matthewdzmura]

Can we assume that users will only ever download models using the download tool? Are we going to want to add any other types of download support to the tool? We currently download datasets from HF as well, though that looks to be done as part of the recipe.

[kartikayk]

@matthewdzmura

Can we assume that users will only ever download models using the download tool

I don't think this is a good assumption. Users can get the model in a variety of ways. Though I think for a tool like download which we advertise as a convenience utility for downloading models from HF, we should remove restrictions that come in the way of doing exactly this.

[matthewdzmura]

My comment wasn't about whether users would download their models from somewhere else, but rather around whether we might want to provide additional download functionality for other types of data e.g. datasets.

I'm in favor of removing model restrictions from the download tool, we can always warn if the user downloads something we don't explicitly support out of the box.

[kartikayk]

want to provide additional download functionality for other types of data e.g. datasets.

Good point, I think our current approach has been to use the load_datasets from HF Datasets. Though IIRC, @joecummings has an open issue of replacing this with the HF Hub (not sure what that entails from a technical perspective).

[joecummings]

@kartikayk Can this be closed?

[kartikayk]

@joecummings yup sounds good. The last bit of clean up with download would be to just log the stuff that we "ignore". I realized this as I was overriding it this weekend. It might not be immediately obvious to folks unless they look at tune download --help. So a simple log of what is being ignored might be good. Otherwise, happy to close this. Thanks so much for the refactor!