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

fix: Changes in function process_dataargs to support the current implementation #2

Conversation

Abhishek-TAMU
Copy link

@Abhishek-TAMU Abhishek-TAMU commented Nov 21, 2024

Description of the change

1- Adding unit test case for testing function process_dataargs with current way of tuning.
2- Changes to make the case of dataset with input, output key work with current way of tuning and make test_process_dataargs_pretokenized handler work.

Related issue number

https://github.ibm.com/ai-foundation/watson-fm-stack-tracker/issues/1428

How to verify the PR

Run current way of tuning for multiple cases:

  • Case with using dataset with text and label as a single sequence, using this dataset

Command used:

python tuning/sft_trainer.py  \
--model_name_or_path Maykeye/TinyLLama-v0  \
--training_data_path tests/data/twitter_complaints_small.jsonl  \
--output_dir outputs/full-tuning  \
--num_train_epochs 5  \
--per_device_train_batch_size 2  \
--gradient_accumulation_steps 1  \
--learning_rate 1e-5  \
--response_template "\n### Label:"  \
--dataset_text_field "output" \
--use_flash_attn false \
--torch_dtype "float32"

Command used:

python tuning/sft_trainer.py  \
--model_name_or_path Maykeye/TinyLLama-v0  \
--training_data_path tests/data/twitter_complaints_tokenized_with_maykeye_tinyllama_v0.jsonl \
--output_dir outputs/full-tuning  \
--num_train_epochs 5  \
--per_device_train_batch_size 2  \
--gradient_accumulation_steps 1  \
--learning_rate 1e-5  \
--use_flash_attn false \
--torch_dtype "float32"

Command used:

python tuning/sft_trainer.py  \
--model_name_or_path Maykeye/TinyLLama-v0  \
--training_data_path tests/data/twitter_complaints_input_output.jsonl  \
--output_dir outputs/full-tuning  \
--num_train_epochs 5  \
--per_device_train_batch_size 2  \
--gradient_accumulation_steps 1  \
--learning_rate 1e-5  \
--use_flash_attn false \
--torch_dtype "float32"

Was the PR tested

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

Copy link

Thanks for making a pull request! 😃
One of the maintainers will review and advise on the next steps.

Comment on lines +41 to +45
fn_kwargs = tokenizer_kwargs.get("fn_kwargs", {})
tokenizer_inner_kwargs = fn_kwargs.get("tokenizer_kwargs", {})

tokenized_comb_seqs = tokenizer(combined, **tokenizer_inner_kwargs)
tokenized_input = tokenizer(input, **tokenizer_inner_kwargs)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference of discussion for this change: foundation-model-stack#381 (comment)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -118,7 +118,7 @@ def process_dataargs(
kwargs = {
"fn_kwargs": fn_kwargs,
"batched": False,
"remove_columns": [JSON_INPUT_KEY, JSON_OUTPUT_KEY],
"remove_columns": "all",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In current implementation, dataset just have these columns input_ids, labels", attention_mask, hence every other columns in the dataset needed to be removed.

@dushyantbehl dushyantbehl merged commit 10d7d66 into dushyantbehl:dataloader-v2-impl Nov 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants