Adds max_seq_len to recipes and updates unit tests

[thomasjpfan]

Context

• add a new feature
• fix a bug
• update tests and/or documentation
• other (Expose new feature in recipe yaml)

Addes task 1 of #1311

Changelog

This PR adds max_seq_len to the recipes and adds unit tests for loading the config

Test plan

Please make sure to do each of the following if applicable to your PR. (If you're not sure about any one of these just ask and we will happily help. We also have a contributing page for some guidance on contributing.)

• run pre-commit hooks and linters (make sure you've first installed via pre-commit install)
• add unit tests for any new functionality
• update docstrings for any new or updated methods or classes
• run unit tests via pytest tests
• run recipe tests via pytest tests -m integration_test
• manually run any new or modified recipes with sufficient proof of correctness
• include relevant commands and any other artifacts in this summary (pastes of loss curves, eval results, etc.)

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1363

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 4eeb1fd with merge base 8bb3a6f ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[RdoubleA]

Sorry for the delay, appreciate you doing this, this certainly improves the usability of all our configs. A few things:

• It seems like the mistral and phi3 configs were missed entirely? Any reason for that?
• For mistral/7B_full_ppo_low_memory.yaml in particular, we'd probably want to change this line:

  torchtune/recipes/configs/mistral/7B_full_ppo_low_memory.yaml

  Line 36 in f9f75bb

  max_seq_len: null

  to be consistent with the classifier, where max_seq_len=32768 (cc @SalmanMohammadi)

[RdoubleA]

max_seq_length is already defined below and is used instead of the one with the tokenizer. To be consistent, let's keep the max_seq_len here in the tokenizer, set it to 4096, remove the other one, and update the eleuther_eval recipe to use the tokenizer's max_seq_len. You simply would need to update the line here:

torchtune/recipes/eleuther_eval.py

Line 246 in f9f75bb

max_seq_length=self._cfg.max_seq_length,

to use the tokenizer max_seq_len: self._cfg.tokenizer.max_seq_len

[SalmanMohammadi]

* For `mistral/7B_full_ppo_low_memory.yaml` in particular, we'd probably want to change this line: https://github.com/pytorch/torchtune/blob/f9f75bb563ecae371492a9d49da4a9f514c081b3/recipes/configs/mistral/7B_full_ppo_low_memory.yaml#L36
   to be consistent with the classifier, where max_seq_len=32768 (cc @SalmanMohammadi)

Text completion datasets don't defer to the tokenizer for max seq length truncation right? If this is the case it's fine to leave as null - unless I'm missing something re why it should be consistent with the classifier?

[RdoubleA]

Hi @thomasjpfan, let us know if you still plan to carry this through or if we should wrap it up. Appreciate your work so far!

[thomasjpfan]

It seems like the mistral and phi3 configs were missed entirely? Any reason for that?

I had a quick script to automate adding max_seq_len and missed mistral & phi3 😅

From #1363 (comment), I kept the tokenizer in mistral/7B_full_ppo_low_memory.yaml to max_seq_len: null.

[RdoubleA]

Looks great! Just one comment and it's ready to go

[RdoubleA]

Appreciate you adding this test, but actually don't think this is necessary. This is simply testing that an object can be instantiated and has the correct parameters, but this is already covered by the other tests in this directory

[thomasjpfan]

I added the test to mostly check for the null case, which we couldn't do with RMSNorm.

In 4eeb1fd (#1363), I simplified the test to only check the null case.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.06%. Comparing base (8bb3a6f) to head (f5da0ef).
Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1363      +/-   ##
==========================================
- Coverage   72.33%   70.06%   -2.28%     
==========================================
  Files         269      268       -1     
  Lines       12668    12936     +268     
==========================================
- Hits         9164     9064     -100     
- Misses       3504     3872     +368     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.