8000 Ensure user sets backend to local w/ quantization by Infernaught · Pull Request #3524 · ludwig-ai/ludwig · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Ensure user sets backend to local w/ quantization #3524

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 11, 2023
Merged

Ensure user sets backend to local w/ quantization #3524

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
Contributor

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

@arnavgarg1 arnavgarg1 mentioned this pull request Aug 11, 2023
Closed
arnavgarg1
arnavgarg1 approved these changes Aug 11, 2023
View reviewed changes
Copy link
8000 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
arnavgarg1 pushed a commit that referenced this pull request Aug 11, 2023
@Infernaught @arnavgarg1
Ensure user sets backend to local w/ quantization (#3524)
420ef80
@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.

tgaddair added a commit that referenced this pull request Aug 12, 2023
@tgaddair
Revert "Ensure user sets backend to local w/ quantization (#3524)"
2f7a17b 
This reverts commit 190f208.
tgaddair added a commit that referenced this pull request Aug 12, 2023
@tgaddair
Revert "Ensure user sets backend to local w/ quantization (#3524)" (#…
5ba3494 
…3531)
tgaddair added a commit that referenced this pull request Aug 12, 2023
@tgaddair
Revert "Ensure user sets backend to local w/ quantization (#3524)" (#…
6fe507c 
…3531)
justinxzhao pushed a commit that referenced this pull request Aug 15, 2023
@tgaddair @justinxzhao
Revert "Ensure user sets backend to local w/ quantization (#3524)" (#…
c369054 
…3531)
tgaddair added a commit that referenced this pull request Aug 15, 2023
@tgaddair
Revert "Ensure user sets backend to local w/ quantization (#3524)" (#…
90b8e4e 
…3531)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnavgarg1 arnavgarg1 arnavgarg1 approved these changes

@justinxzhao justinxzhao Awaiting requested review from justinxzhao justinxzhao is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Infernaught @justinxzhao @tgaddair @arnavgarg1
0