Add documentation on adding new recipe params #311

RdoubleA · 2024-02-06T00:14:58Z

Context

We need to provide users guidance on how params for new recipes should be added and integrate with the existing dataclass/config system. I've added it as a tutorial. In a follow up, we should consider how to consolidate this tutorial and the recipe tutorial in #316

Addresses #283

Test plan

netlify · 2024-02-06T00:15:03Z

✅ Deploy Preview for torchtune-preview ready!

Name	Link
🔨 Latest commit	`b77213a`
🔍 Latest deploy log	https://app.netlify.com/sites/torchtune-preview/deploys/65c57aef6862af000845e650
😎 Deploy Preview	https://deploy-preview-311--torchtune-preview.netlify.app/examples/configs
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

RdoubleA · 2024-02-06T00:15:39Z

recipes/README.md

+```
+In the dataclass, all fields should have defaults assigned to them. If a reasonable value cannot be assigned or it is a required argument, use the null value for that data type as the default and ensure that it is set by the user in the `__post_init__` (see Parameter Validation). The dataclass should go in the `recipes/params/` folder and the name of the file should match the name of the recipe file you are creating.
+
+To link the dataclass object with config and CLI parsing, you can use the `TuneArgumentParser` object and funnel the parsed arguments into your dataclass.


TODO: make this a helper function so it's much easier to set up in any recipe

RdoubleA · 2024-02-06T00:16:14Z

recipes/README.md

+config_to_params = {
+    os.path.join(ROOT_DIR, "alpaca_llama2_full_finetune.yaml"): FullFinetuneParams,
+    ...,
+}


TODO: figure out how to automate this

NicolasHug · 2024-02-06T09:50:15Z

@RdoubleA do you need a review or is this still [WIP] (as the title prefix suggests)

joecummings · 2024-02-06T15:14:50Z

recipes/README.md

@@ -53,3 +53,60 @@ To run the generation recipe, run this command from inside the main `/torchtune`
 ```
 python -m recipes.alpaca_generate --native-checkpoint-path /tmp/finetune-llm/model_0.ckpt --tokenizer-path ~/llama/tokenizer.model --input "What is some cool music from the 1920s?"
 ```
+
+&nbsp;


This is good for now maybe, but I would push for this to be in our actual documentation, not the README.

RdoubleA · 2024-02-06T21:06:16Z

apologies for the confusion, this was pending some discussions around where to put tutorials and interactions with other tutorials. It is ready for review now @NicolasHug @kartikayk

joecummings · 2024-02-06T21:38:40Z

docs/source/examples/recipe_params_tutorial.rst

@@ -0,0 +1,68 @@
+======================================
+Creating parameters for custom recipes


This feels so specific and without much context. Could this be part of a "Create a custom recipe" tutorial? On it's own, I don't know if it stands.

yeah I agree, I think the plan is to merge with @kartikayk 's tutorial once we're both checked in

In that case, I love it!

Not sure I understand the reasoning of checking both in, then combining. Would it make more sense to wait for Kartikay to get his checked in, then rebase and add this stuff on?

why not the other way around :)

ya you're right that's simpler, there just seemed to be a lot of discussion on the recipe tutorial that I might as well get this in first. I'm ok to wait and rebase too, either works

kartikayk · 2024-02-06T21:46:02Z

docs/source/examples/recipe_params_tutorial.rst

+Creating parameters for custom recipes
+======================================
+
+In general, you should expose the minimal amount of parameters you need to run and experiment with your recipes. These should be collected in a dataclass object that is passed into the recipe.


This sounds a bit arbitrary without the reasoning i.e. you want to be thoughtful of what you expose because a) not everything is actually configured and b) you don't want to bloat configs since bloated configs are more error prone (typos, untested combinations of params etc), harder to read and harder to manage

yeah, I didn't want to go too much into the philosophy of what to expose in configs. but since users are the one designing it, it might be useful to share these details

Also a line capturing the reasoning around why we have a dataclass i.e. single source of truth, easier to validate etc.

kartikayk · 2024-02-06T21:52:36Z

docs/source/examples/recipe_params_tutorial.rst

+
+Write config
+------------
+Now that you've set up the recipe, the parameters dataclass, and the parser, you can create a simple config in :code:`recipes/configs/` that specifies values for any of the fields you defined in the dataclass. Anything that is not specified should have a default value in the dataclass.


Maybe - "Let's assume you already have a recipe and want to add a config".

kartikayk · 2024-02-06T21:53:43Z

docs/source/examples/recipe_params_tutorial.rst

+
+Testing configs
+---------------
+TorchTune has testing for every config added to the library, namely ensuring that it instantiates the dataclass and runs the recipe correctly. To add your config to this test suite, simply update the dictionary in :code:`recipes/tests/configs/test_configs.py`.


Users don't need to do this for configs which wont be checked in? So maybe prefix this with, "if you plan on adding a PR for landing this in the repo" or something like that

ebsmothers · 2024-02-08T18:56:59Z

docs/source/examples/recipe_deepdive.rst

@@ -54,35 +54,25 @@ What Recipes are not?
 - **Wrappers around external frameworks.** A recipe is **not** meant to be a wrapper around external frameworks. These are fully written in native-PyTorch using TorchTune building blocks. Dependencies are primarily in the form of additional utilities or interoperability with the surrounding ecosystem (eg: EluetherAI's evaluation harness).


while you're here: Eluether -> Eleuther

Guilty as charged

ebsmothers · 2024-02-08T18:59:19Z

docs/source/examples/configs.rst

+---------------
+If you plan on contributing your config to the repo, we recommend adding it to the testing suite. TorchTune has testing for every config added to the library, namely ensuring that it instantiates the dataclass and runs the recipe correctly.
+
+To add your config to this test suite, simply update the dictionary in :code:`recipes/tests/configs/test_configs.py`.


Is there a way to point directly to a variable or something? I can easily imagine something like this getting stale if we ever move things around

ah good point, yes there is

ebsmothers · 2024-02-08T19:00:11Z

docs/source/examples/configs.rst

+
+Testing configs
+---------------
+If you plan on contributing your config to the repo, we recommend adding it to the testing suite. TorchTune has testing for every config added to the library, namely ensuring that it instantiates the dataclass and runs the recipe correctly.


lol i didn't even know this

Tutorial already doing its job.. #353

r u even a torchtune dev

ebsmothers · 2024-02-08T19:01:22Z

docs/source/examples/configs.rst

+Command-line overrides
+----------------------
+To enable quick experimentation, you can specify override values to parameters in your config
+via the :code:`tune` command. These should be specified with the flag :code:`--override k1=v1 k2=v2 ...`


Personally I would give an explicit example here, I screwed this up the first few times I tried it. Might also be worth making this section a bit more visible tbh

ebsmothers · 2024-02-08T19:10:17Z

docs/source/examples/configs.rst

+Defining parameters for custom recipes
+--------------------------------------
+
+In general, you should expose the minimal amount of parameters you need to run and experiment with your recipes.


So this is a nit cause overall I think this tutorial is pretty well-structured. But I feel like you are leading with config best practices when it may be better to lead with "how can I get something simple running". Only after that should you tell someone "hey now that you're ready to write your own stuff, here are best practices you should keep in mind"

ebsmothers · 2024-02-08T19:13:24Z

docs/source/examples/configs.rst

+        logger = utils.get_logger("DEBUG")
+        logger.info(msg=f"Running finetune_llm.py with parameters {params}")


I know it's not part of this PR per se, but do we plan to do something like this by default eventually?

ebsmothers · 2024-02-08T19:15:41Z

docs/source/examples/configs.rst

+
+.. code-block:: bash
+
+    tune <recipe> --config <config> --override ...


Doesn't it also need to be added here first? Maybe I'm missing something obvious

I didn't know about this. I'll update the tutorial for now but we should revisit this design

kartikayk · 2024-02-08T20:40:44Z

@RdoubleA seems like some duplication since "Finetune your first LLM" appears twice. We should remove this from under tutorials.

ebsmothers · 2024-02-08T22:02:25Z

docs/source/examples/configs.rst

+
+There are two primary entry points for users to configure parameters: configs and
+CLI overrides. Configs are YAML files that define all the parameters that the user
+wishes to run a recipe with in a single location. These can be overridden on the


nit

Suggested change

wishes to run a recipe with in a single location. These can be overridden on the

wishes to run a recipe within a single location. These can be overridden on the

Oh wait maybe ignore this.. I realized the with comes from "the user wishes to run a recipe with". But personally I would still change the phrasing here, why not "all the parameters needed to run a recipe", or something like that?

kartikayk · 2024-02-09T00:59:44Z

docs/source/examples/configs.rst

+      * Understand the :ref:`fundamentals of recipes<recipe_deepdive>`
+
+
+Where parameters are sourced


Sourced here reads really weirdly. Maybe "Where do parameters live?"

docs/source/examples/configs.rst

Co-authored-by: Kartikay Khandelwal <47255723+kartikayk@users.noreply.github.com>

kartikayk · 2024-02-09T01:03:29Z

docs/source/examples/configs.rst

+Configs serve as the primary entry point for running recipes in TorchTune. They are
+expected to be YAML files and simply list out values for parameters you want to define
+for a particular run. The config parameters should be a subset of the dataclass parameters;
+there should not be any new fields that are not in the dataclass. Any parameters that


Suggested change

there should not be any new fields that are not in the dataclass. Any parameters that

there should not be any new fields that are not already in the dataclass. Any parameters that

kartikayk

Some nits and suggestions, but overall looks good. Thanks for adding this!

rohan-varma · 2024-02-09T01:19:49Z

docs/source/examples/configs.rst

+        params = FullFinetuneParams(**args)
+
+        logger = utils.get_logger("DEBUG")
+        logger.info(msg=f"Running finetune_llm.py with parameters {params}")


its now called full finetune

rohan-varma · 2024-02-09T01:20:42Z

docs/source/examples/configs.rst

+        logger = utils.get_logger("DEBUG")
+        logger.info(msg=f"Running finetune_llm.py with parameters {params}")
+
+        recipe(params)


We call recipe(params), but I don't see where recipe is defined / imported. Is this expected to be executable?

nope, it's meant to be a placeholder for the main function of the user's recipe. can add a comment to clarify that in a follow up, since this whole code chunk could change after #329

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 6, 2024

RdoubleA commented Feb 6, 2024

View reviewed changes

RdoubleA requested review from kartikayk, NicolasHug and rohan-varma February 6, 2024 00:22

joecummings reviewed Feb 6, 2024

View reviewed changes

RdoubleA mentioned this pull request Feb 6, 2024

Recipe tutorial + TorchTune Overview #316

Merged

RdoubleA changed the title ~~[WIP] Add documentation on adding new recipes~~ [WIP] Add documentation on adding new recipe params Feb 6, 2024

RdoubleA changed the title ~~[WIP] Add documentation on adding new recipe params~~ Add documentation on adding new recipe params Feb 6, 2024

joecummings reviewed Feb 6, 2024

View reviewed changes

kartikayk reviewed Feb 6, 2024

View reviewed changes

RdoubleA force-pushed the rafiayub/config_documentation branch 3 times, most recently from d90c067 to ab1c112 Compare February 8, 2024 18:22

ebsmothers reviewed Feb 8, 2024

View reviewed changes

RdoubleA force-pushed the rafiayub/config_documentation branch from 1751fb2 to 0444e6f Compare February 8, 2024 20:08

ebsmothers reviewed Feb 8, 2024

View reviewed changes

RdoubleA added 12 commits February 8, 2024 16:05

update readme

a4dc06f

focus on dataclasses only

6a6ab45

move to live docs

60cd24a

address review comments

0b30d61

move to recipe tutorial

74e1f5c

typos

fa2c16b

move to config tutorial

c072514

update configs tutorial

b6e3a38

link tutorials and fix typos

a3bce8d

address comments

5911ff2

remove duplicate tutorial

b9e7bd1

address comments, add some links

f5e4ecf

RdoubleA force-pushed the rafiayub/config_documentation branch from fe93b53 to f5e4ecf Compare February 9, 2024 00:20

kartikayk reviewed Feb 9, 2024

View reviewed changes

docs/source/examples/configs.rst Outdated Show resolved Hide resolved

kartikayk reviewed Feb 9, 2024

View reviewed changes

docs/source/examples/configs.rst Outdated Show resolved Hide resolved

kartikayk reviewed Feb 9, 2024

View reviewed changes

docs/source/examples/configs.rst Outdated Show resolved Hide resolved

kartikayk reviewed Feb 9, 2024

View reviewed changes

docs/source/examples/configs.rst Outdated Show resolved Hide resolved

RdoubleA and others added 4 commits February 8, 2024 17:02

Update docs/source/examples/configs.rst

49ec397

Co-authored-by: Kartikay Khandelwal <47255723+kartikayk@users.noreply.github.com>

Update docs/source/examples/configs.rst

69d31b0

Co-authored-by: Kartikay Khandelwal <47255723+kartikayk@users.noreply.github.com>

Update docs/source/examples/configs.rst

a3daea7

Co-authored-by: Kartikay Khandelwal <47255723+kartikayk@users.noreply.github.com>

Update docs/source/examples/configs.rst

bc938b4

Co-authored-by: Kartikay Khandelwal <47255723+kartikayk@users.noreply.github.com>

kartikayk reviewed Feb 9, 2024

View reviewed changes

kartikayk approved these changes Feb 9, 2024

View reviewed changes

address final comments

b77213a

RdoubleA merged commit 1da83d6 into main Feb 9, 2024
15 checks passed

rohan-varma reviewed Feb 9, 2024

View reviewed changes

RdoubleA deleted the rafiayub/config_documentation branch February 9, 2024 01:22

kartikayk mentioned this pull request Feb 25, 2024

Add documentation on how users add new parameters to existing recipes or create new recipes #283

Closed

		@@ -0,0 +1,68 @@
		======================================
		Creating parameters for custom recipes

		@@ -54,35 +54,25 @@ What Recipes are not?
		- Wrappers around external frameworks. A recipe is not meant to be a wrapper around external frameworks. These are fully written in native-PyTorch using TorchTune building blocks. Dependencies are primarily in the form of additional utilities or interoperability with the surrounding ecosystem (eg: EluetherAI's evaluation harness).

		logger = utils.get_logger("DEBUG")
		logger.info(msg=f"Running finetune_llm.py with parameters {params}")


		.. code-block:: bash

		tune <recipe> --config <config> --override ...

	wishes to run a recipe with in a single location. These can be overridden on the
	wishes to run a recipe within a single location. These can be overridden on the

		* Understand the :ref:`fundamentals of recipes<recipe_deepdive>`


		Where parameters are sourced

	there should not be any new fields that are not in the dataclass. Any parameters that
	there should not be any new fields that are not already in the dataclass. Any parameters that

Add documentation on adding new recipe params #311

Add documentation on adding new recipe params #311

Conversation

RdoubleA commented Feb 6, 2024 • edited

Context

Test plan

netlify bot commented Feb 6, 2024 • edited

✅ Deploy Preview for torchtune-preview ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug commented Feb 6, 2024

Choose a reason for hiding this comment

RdoubleA commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikayk commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikayk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RdoubleA commented Feb 6, 2024 •

edited

netlify bot commented Feb 6, 2024 •

edited