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

Better error raised when cloned without lfs #13401

Merged
merged 2 commits into from
Sep 8, 2021
Merged

Conversation

LysandreJik
Copy link
Member

Raise a better error when cloning a repository without having git-lfs installed.

The pointer files contain data such as the following:

version https://git-lfs.github.com/spec/v1
oid sha256:c393f632913cdc64c13fcd1b039a74f17dd83cc3029c556802c0f2f8792b46f9
size 557941479

This tries to parse the file and check that the first string is version; if so, it detects it as a pointer file.

Closes #8497

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Looking good! I've left some comments on the syntax: in general we should chain exceptions better by using the from error syntax (cf internal discussion with Sylvain Lesage in slack recently).

"you cloned."
)
else:
raise ValueError
Copy link
Collaborator

Choose a reason for hiding this comment

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

No message in the error? Also it should be from e with e being the previous error caught by the first except. This makes it easier to see the original source of an error if someone has some try except around our code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't put a message in the error because this raise ValueError is here simply to be caught by the try/assert around it:

except (UnicodeDecodeError, ValueError):

This ensures the exact same error as before is raised from a non-git-lfs weight file:

raise EnvironmentError(f"Unable to convert {archive_file} to Flax deserializable object. ")

See previous error:

                except UnpicklingError:
                    raise EnvironmentError(f"Unable to convert {archive_file} to Flax deserializable object. ")

else:
raise ValueError
except (UnicodeDecodeError, ValueError):
raise EnvironmentError(f"Unable to convert {archive_file} to Flax deserializable object. ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for the from syntax here.

@LysandreJik LysandreJik merged commit 99029ab into master Sep 8, 2021
@LysandreJik LysandreJik deleted the git-lfs-error-raise branch September 8, 2021 12:28
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.

Error when loading a model cloned without git-lfs is quite cryptic
3 participants