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

Updated one_click.py #4994

Merged
merged 15 commits into from
Jan 5, 2024
Merged

Updated one_click.py #4994

merged 15 commits into from
Jan 5, 2024

Conversation

matthewraaff
Copy link
Contributor

Hello, I'm opening this pull request to change the following:

  • Add auto-detection of GPU
  • Add GPU enum
  • More dynamic requirement function
  • Automated addition of the --cpu flag

This should make it a slightly nicer experience for people installing.

When running pyflakes, I did receive the following recommendation, however it was there before. "one_click.py:282:5: local variable 'is_intel' is assigned to but never used". I don't think this should be problematic.

Checklist:

Add auto-detection of GPU
Add GPU enum
More dynamic requirement function
Automated addition of the --cpu flag
I added newlines at the end of my new functions to fix pycodestyle problems
one_click.py Outdated
print("N) None (I want to run models in CPU mode)")
print()

choice = input("Input> ").upper()
while choice not in 'ABCDN':
while choice not in 'ABCDE':
Copy link
Owner

Choose a reason for hiding this comment

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

Missed an N here

@oobabooga
Copy link
Owner

Your changes are excellent. My only concern is that there may be edge cases where the auto-select option does not work correctly, like:

  1. The detection commands fail to work on some system for some arbitrary reason.
  2. False-positives.
  3. Systems with two GPUs with mixed brands.

What are your thoughts on this?

When running pyflakes, I did receive the following recommendation, however it was there before. "one_click.py:282:5: local variable 'is_intel' is assigned to but never used". I don't think this should be problematic.

This was added for when Intel Arc gets its own requirements.txt file. In fact it probably should use requirements_cpu_only.txt / requirements_cpu_only_noavx2.txt since there are not any Intel wheels available. Otherwise this line can just be commented out.

I made some additional changes to the one-click installer at #5056 and your review would be welcome if you are interested.

@matthewraaff
Copy link
Contributor Author

matthewraaff commented Dec 23, 2023

Hello, thank you for your concise review.

I'll make some changes to the code soon, I think we can address the first concern of failure with this by introducing exception catching.

False positives may be of concern, but I don't think they should be too prevalent with the current gpu releases. The windows command works by using wmic to get the full name of the GPU, for example NVIDIA GeForce RTX 3070. If the model name somehow contains the name of another manufacturer, we could resolve this by checking what comes up first in the string.

I don't have an apple device to test on, but I tested the lspci command on another linux device, and found an issue relating to false positives. It doesn't just report the GPU, but also the audio device. Example below.

matthew@ubuntu:~$ lspci | grep -i amd
01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Caicos                                                                              [Radeon HD 6450/7450/8450 / R5 230 OEM]

01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Caicos HDMI Audio [                                                                             Radeon HD 6450 / 7450/8450/8490 OEM / R5 230/235/235X OEM]

This was unexpected, and I wasn't able to get it from my previous testing. I will mitigate this by using the command lspci -vnn | grep -i 'VGA\|3D\|Display', which uses grep and a pipe to filter only lines that contain "VGA", "3D" or "Display", lowering the chances of false positives on linux. If this is not able to detect anything, we will fallback to the old command which is reliable for most cases, or ask the user to select.

When multiple GPUs are detected, it will detect in the order of Nvidia, AMD, Intel. In the future, we may be able to find which one is most powerful, but for now it should be alright doing it in this order.

#5056 lgtm :) - i'm new to open source contributions so I'm not sure how to merge it into this pr.

The is_intel function shouldn't be problematic, I'll comment it out in the next series of commits, feel free to reverse that.

Expect the changes/fixes to be made within the next hour or so. Thanks!

matthewraaff and others added 6 commits December 23, 2023 15:08
This allows us to run the command once instead of up to 3 times.
- now we ignore the recommendations made that we're told to ignore in the contributing guidelines.
- fixed E226 on line 480
@oobabooga oobabooga changed the base branch from main to dev January 5, 2024 02:09
@oobabooga
Copy link
Owner

As much as the GPU autodetection code is correct, I will opt to not include it because

  1. The GPU choice is usually an obvious for the user.
  2. It adds complexity to the codebase.

The remaining changes are excellent:

  • Adding the --cpu flag automatically
  • Simplifying the syntax for getting the requirements.txt
  • Referring to the GPU by name rather than by choice == "A"
  • Adding a setup.cfg for pycodestyle

I highly appreciate those changes. Thank you for this PR!

@oobabooga oobabooga merged commit c9c31f7 into oobabooga:dev Jan 5, 2024
PoetOnTheRun pushed a commit to PoetOnTheRun/text-generation-webui that referenced this pull request Feb 22, 2024
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.

2 participants