adds early-out for model type checking based on path type - #1791

removing this causes test to break because this is the only file that the test checks for.

Also the repo tends to do: logger.debug(f"Message: {var}") for formatting.

Linter wants me to use lazy logging via %s rather than f-strings

lazy linter! let's stay consistent :)

Okay no problem, just changed over. In the future we may want to standardize on lazy logging instead- I looked into it a bit and it seems to be a bit faster.

Just as a note - if a Path type isn't passed here, this still throw an exception as is_dir() is a method of the Path class: https://kodify.net/python/check-path-is-directory/#use-pathis_dir-or-ospathisdir

Not blocking, but you may also consider os.path.isdir() - the differences in behavior are outlined in that link above.

@nathan-weinberg updated

Looks good, though this will throw an exception if that casting fails

Wouldn't NotADirectoryError handler catch the situation? Why do we need to handle this explicitly here, again?

With my suggestion above, we ensure a Path type is being passed, so agreed with @booxter here

I think you're right @booxter. Explicitly checking if a path is a directory above is redundant since we catch an informative exception. I'll make a change reflective of that.

Similar story here to my comment above: https://www.freecodecamp.org/news/how-to-check-if-a-file-exists-in-python/

@nathan-weinberg updated

Same comment as above

Same code snippet suggestion here, given the above conversation

if not isinstance(model_path, pathlib.Path):
    raise TypeError("'model_path' must be of type 'pathlib.Path')

Then below, can do something similar to the safetensors func

try:
    with open(model_path, "rb") as f:
        # Memory-map the file on the first 4 bytes (this is where the magic number is)
        mmapped_file = mmap.mmap(f.fileno(), length=4, access=mmap.ACCESS_READ)
        # Read the first 4 bytes
        first_four_bytes = mmapped_file.read(4)
        # Convert the first four bytes to an integer
        first_four_bytes_int = int(struct.unpack("<I", first_four_bytes)[0])
        # Close the memory-mapped file
        mmapped_file.close()
        return first_four_bytes_int == GGUF_MAGIC
except (FileNotFoundError, PermissionError) as e:
    logger.debug("Failed to read file: %s", e)
    return False

@booxter let me know your thoughts here as well ^^

I disagree with the premise of this change - assuming callers may pass something but a Path. Instead of adding ad-hoc conversion code like proposed here, the callers should be forced to pass the expected types, if not already. The latter can also be enforced by adding more typing checks up the call stack.

Otherwise, why not handling model_path being a None, or some other type? This is a slippery slope.

Definitely see your point. We can drop this if it's truly a bad route. It's a modest degree of leniency in this codebase because there's some heterogeneity on path handling- some modules might want to call this method that use str's representing paths. Since those representations are very close, I'm inclined to massage the input into a format that this function wants. Otherwise, if it's not something that's reasonably handle-able, it's rejected because it's NotADirectory.

Elsewhere, we were ripping off some of these "special handling for string paths", so I'd like to avoid this push-pull. The strategic direction I think is to convert all code that attempts to pass strings where Paths are expected to actually pass Paths. (Same for other types.) These are code bugs, so they should be fixed in-place, not worked around.

I propose the following snippet

if not isinstance(model_path, pathlib.Path):
    raise TypeError("'model_path' must be of type 'pathlib.Path')

This remains in the spirit of my comment, James's implementation, and Ihar's feedback.

I thought a bit about this offline- I think @booxter's route of trusting the function definition (that the input is a pathlib.Path) is the right way to go, otherwise we'd be writing input type-validation code for every function. We're using type annotations so anyone who passes a str to this method should be warned by their development environment that this isn't a good way to do things.

Wouldn't NotADirectoryError handler catch the situation? Why do we need to handle this explicitly here, again?

@JamesKunstle I think this change may actually be useful, since right now if a directory is passed, the function will raise an exception (on open). The rest of changes here, as you confirmed, are probably not a good idea. (Thanks.)

An alternative to is_file check could be catching misc exceptions expected from open, and returning False on any of them.