Check for tokenizer in downloaded models directory

[khaledsulayman]

This change forces tokenizer loading to be done using the locally downloaded teacher model, as opposed to pulling from huggingface.

Resolves: #343

Signed-off-by: Khaled Sulayman [email protected]

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. @khaledsulayman please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. @khaledsulayman please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[RobotSail]

It seems like we're including artifacts directly in our code for testing purpose, e.g.: tests/testdata/models/instructlab/granite-7b-lab/tokenizer.json.

We want to avoid this approach since the artifacts themselves may become out-of-date overtime and therefore lead to inaccurate tests, in addition to adding an unnecessary amount of data to our codebase. If possible, I recommend to instead setup a workflow in which these files are downloaded automatically as part of the test suite. The AutoTokenizer.from_pretrained API already does this when loading the tokenizer, so I would check if there's a more direct way to simply download the tokenizer.

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. @khaledsulayman please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[khaledsulayman]

@RobotSail I see your point. I personally don't feel too strongly either way, I'll let others chime in on if that's worth blocking this PR. I've also opened #384 so we don't lose track of it

[aakankshaduggal]

Thanks @khaledsulayman! LGTM 🚀

[bbrowning]

This looks good enough to me to merge and then keep improving things as we go. A few things that come to mind we may want to tackle later:

• renaming tokenizer_model_name to something else, since it really expects a directory and not a huggingface model name
• consider factoring out some common requirements, util classes, etc into some instructlab/common repo/package that SDG, CLI, and training repos could share - things like is_model_gguf or ensuring we're all aligned on identical versions of transformers, torch, etc - needs more discussion outside this PR, but this PR copies a few more things from instructlab/instructlab that we'll want to stay in-sync over time
• adding an e2e test that exercises the ContextAwareChunker - right now we have a functional test that tests PDF chunking, but we don't have something that ensures the entire e2e flow from ilab data generate is wired up properly for PDFs, including things like the tokenizer_model_name gets passed in as expected

Because we're hoping to get a release out soon and some of us are off tomorrow, I don't think any of the above needs to hold this up. And, some of the above would explode the scope of this PR so best done separately anyway.

Thanks for all the work on this!