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

Ensure user sets backend to local w/ quantization #3524

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

Infernaught
Copy link
Contributor

Previously: users could omit backend from their config when running LLM finetuning with quantization

Now: users are prompted to set backend to local explicitly when running LLM finetuning with quantization

@github-actions
Copy link

Unit Test Results

       6 files  ±       0         6 suites  ±0   1h 39m 15s ⏱️ + 21m 10s
2 812 tests  -        4  2 788 ✔️  -      15  21 💤 +  9  3 +2 
8 401 runs  +5 542  8 338 ✔️ +5 501  54 💤 +33  9 +8 

For more details on these failures, see this check.

Results for commit 0bc61a3. ± Comparison against base commit 3691dbe.

@justinxzhao
Copy link
Collaborator

The change LGTM. It looks like there's some failing test. @Infernaught can you take a look?

Copy link
Contributor

@arnavgarg1 arnavgarg1 left a comment

Choose a reason for hiding this comment

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

LGTM

@arnavgarg1 arnavgarg1 merged commit 190f208 into ludwig-ai:master Aug 11, 2023
15 checks passed
@tgaddair
Copy link
Collaborator

This is very aggressive check that is already causing user-facing issues (#3529).

I understand the motivation for raising an error if the user selects quantization and the Ray backend, but that's not what this is doing. This is raising an error if the user doesn't EXPLCITLY select the local backend, which is quite extreme.

We should NEVER require user to specify the local backend explicitly if the local backend is going to be used anyway. Going to need to revert this and yank the v0.8.1 release.

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.

None yet

4 participants