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

scripts: Use local gguf package when running from repo #2927

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

KerfuffleV2
Copy link
Collaborator

You can get the default behavior by setting the NO_LOCAL_GGUF environment variable.

Personally I find the new behavior of ignoring the in-repo version of GGUF kind of unintuitive. This simple pull just adds the local gguf to sys.path if it exists and NO_LOCAL_GGUF is unset. That way one can use the gguf version that's in sync with the scripts without having to worry about setting up environments or manually syncing.

I didn't apply this to the convert-llama scripts because of #2906

Copy link
Collaborator

@monatis monatis left a comment

Choose a reason for hiding this comment

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

a better workaround than symlinking

@KerfuffleV2 KerfuffleV2 requested a review from cebtenzzre August 31, 2023 12:16
@KerfuffleV2
Copy link
Collaborator Author

I'm gonna need a second opinion that I don't suck.

@ghost
Copy link

ghost commented Aug 31, 2023

Related: #2697 (comment)

I had an issue previously, and I think this PR would've solved it (maybe even without my noticing). Thanks in general!

Copy link
Collaborator

@cebtenzzre cebtenzzre left a comment

Choose a reason for hiding this comment

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

Could the paths be made relative to __file__ so you can run the scripts from any directory, and so we don't have to check whether gguf-py exists?
Also, 'NO_LOCAL_GGUF' not in os.environ is a more typical way of writing the env variable check.

@KerfuffleV2
Copy link
Collaborator Author

KerfuffleV2 commented Aug 31, 2023

Sounds reasonable. Is Path(__file__).parent.joinpath('gguf-py', 'gguf', '__init__.py') a reasonable way to make it relative to __file__ or is there nicer way to do it?

edit:

# Use local gguf module if available.
if 'NO_LOCAL_GGUF' not in os.environ and (Path(__file__).parent.absolute().joinpath('gguf-py', 'gguf', '__init__.py')).is_file():
    sys.path.insert(1, str(Path(__file__).parent.absolute().joinpath('gguf-py', 'gguf')))

@KerfuffleV2 KerfuffleV2 requested a review from cebtenzzre August 31, 2023 18:32
@cebtenzzre
Copy link
Collaborator

I don't see why we're using .absolute(), and I also don't see a reason to check if the file exists. If the user has locally deleted the 'gguf-py' folder or removed '__init__.py', there is still no harm in adding it to sys.path anyway - it will just be ignored.

In other words, I think we should just do this (with additional '..' where needed):

if 'NO_LOCAL_GGUF' not in os.environ:
    sys.path.insert(1, str(Path(__file__).parent / 'gguf-py' / 'gguf'))

@KerfuffleV2
Copy link
Collaborator Author

I don't see why we're using .absolute()

Sure, that's no problem. I was just trying to be careful and avoid any potential weirdness, especially since I don't know exactly how stuff is going to work on Windows.

and I also don't see a reason to check if the file exists.

Just a habit I acquired (the hard way) of checking conditions only doing work if necessary. Makes it less likely for strange stuff to happen in the future. In this particular case it's very unlikely to cause a problem though.

@KerfuffleV2 KerfuffleV2 merged commit aeefac4 into ggerganov:master Aug 31, 2023
@KerfuffleV2 KerfuffleV2 deleted the feat-scripts-use-local-gguf branch September 6, 2023 08:51
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