Skip to content

Separate trust_remote_code args #152

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

Merged
merged 8 commits into from
Sep 12, 2024
Merged

Conversation

kylesayrs
Copy link
Collaborator

@kylesayrs kylesayrs commented Sep 9, 2024

Purpose

  • Allow users to trust remote code of datasets
  • Fix some of the finetune tests (all except tests/llmcompressor/transformers/finetune/test_oneshot_then_finetune)

Changes

  • Renamed ModelArgs.trust_remote_code to ModelArgs.trust_remote_code_model to avoid name conflict during arg parsing
  • Added DataTrainingArgs.trust_remote_code_data
  • Added trust_remote_code_data arg to tests

Testing

  • Removed dataset cache and ran python3 -m pytest tests/llmcompressor/transformers/finetune

Copy link

github-actions bot commented Sep 9, 2024

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

@dsikka
Copy link
Collaborator

dsikka commented Sep 10, 2024

This LGTM and seems to only impact the finetune pathway. One suggestion @kylesayrs is to checkout a branch of https://github.com/neuralmagic/llm-compressor-testing/tree/main/.github/workflows and update such that instead of main, we checkout this branch. Can then kick off the weekly and nightly testing to make sure this doesn't cause any other impacts on args. Let me know if this is unclear. The tests should finish in about an hour each

@kylesayrs kylesayrs marked this pull request as draft September 10, 2024 01:06
@kylesayrs kylesayrs marked this pull request as ready for review September 10, 2024 01:12
@kylesayrs
Copy link
Collaborator Author

Initial tests were using a stale cache. In order to clear the HF dataset caches, you must remove two directories

rm -r ~/.cache/huggingface/datasets/*
rm -r ~/.cache/huggingface/modules/datasets_modules/datasets/

@dsikka dsikka requested a review from Satrat September 11, 2024 16:24
Copy link
Contributor

@Satrat Satrat left a comment

Choose a reason for hiding this comment

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

LGTM, but agreed with Dipika lets run the nightly and weekly tests on this change before merging since we're changing argument names

@kylesayrs
Copy link
Collaborator Author

kylesayrs commented Sep 12, 2024

nightly
weekly
e2e

@kylesayrs kylesayrs requested review from dsikka and Satrat September 12, 2024 13:52
@dsikka dsikka merged commit d37b52d into main Sep 12, 2024
6 of 7 checks passed
@dsikka dsikka deleted the kylesayrs/fix-finetune-testing branch September 12, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants