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.

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

Adds package and requirement spec output to version check exception #18702

Merged
merged 3 commits into from
Sep 15, 2022
Merged

Adds package and requirement spec output to version check exception #18702

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
783e436
Adds package and requirement spec output to version check exception
colindean Aug 19, 2022
0d0b89c
Combines version presence check and streamlines exception message
colindean Aug 24, 2022
eef2e7f
Merge remote-tracking branch 'origin/main' into helpful-value-error
stas00 Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/transformers/utils/versions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@

def _compare_versions(op, got_ver, want_ver, requirement, pkg, hint):
if got_ver is None:
raise ValueError("got_ver is None")
raise ValueError(f"got_ver is None when checking {requirement} for {pkg}")
if want_ver is None:
raise ValueError("want_ver is None")
raise ValueError(f"want_ver is None when checking {requirement} for {pkg}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the improvement, @colindean

I'm totally in agreement. But let's make it even better as None has no meaning to the user in the context of internal variable names.

Perhaps something like this?

    if got_ver is None or want_ver is None:
        raise ValueError(f"can't compare need={want_ver} and found={got_ver} versions when checking for {requirement}")

(untested)

I don't think we need pkg since it's already in the requirement.

BTW, I'm curious in what context did you get this triggered? as this was just a defensive programming and shouldn't have really happened. Unless it receives want_ver=None when the package isn't there - it's been a while so I don't quite remember.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make the suggested change and push it up shortly.

I somehow ended up with empty packages installed. As in, the folders and the egg info directory exist, so the package look like it's installed to importlib but then it has no version information (and no files to actually load). It's quite an invalid state but I have no idea how it got to be like that beyond the possibility of arbitrary file deletion: our venv was on a shared system and a junior admin haphazardly deleted all files with creation dates older than 2022-01-01 to free up space… this created a lot of weirdness! However, I'd reinstalled the venv and its packages with Conda after that event, so I'm really not sure what happened. Manually installing the packages with pip brought 'em back. Assumptions about shared filesystems made by my predecessors were… unsafe. And transformers is getting slightly better because of it!

Copy link
Contributor

@stas00 stas00 Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for sharing the details of your use-case, Colin. This is definitely a possibility for users, so yes, I'm glad you are making the Transformers code more user-friendly, especially so when it's already a troublesome situation.

I'm not sure if perhaps it'd help to also add at the end: "you may want to try to force the reinstall of the {pkg} package"

Copy link
Contributor

@stas00 stas00 Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@colindean, currently the autoformatter CI job fails - you need to run make style and push the changes. run pip install -e .[dev] if the former fails due to some dependency missing and try again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to get pip install -e .[dev] to complete successfully.

ERROR: Could not find a version that satisfies the requirement tensorflow-text; extra == "dev" (from transformers[dev]) (from versions: none)
ERROR: No matching distribution found for tensorflow-text; extra == "dev"

I'm on an M1 Mac and there's no whl for macos arm64 for tensorflow-text in Tensorflow 2.9.0. Also, fugasi required installation of mecab from Homebrew… I'll see about adding some hints to CONTRIBUTING about that last one but I don't have a good workaround for the former. Its unavailability blocks installation of packages :-/

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just install the quality tools with pip install -e .[quality], no need for all the dev dependencies.

if not ops[op](version.parse(got_ver), version.parse(want_ver)):
raise ImportError(
f"{requirement} is required for a normal functioning of this module, but found {pkg}=={got_ver}.{hint}"
Expand Down