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

[fixup/style] requires TF but doesn't say that cleanly #11570

Closed
stas00 opened this issue May 3, 2021 · 6 comments · Fixed by #11573
Closed

[fixup/style] requires TF but doesn't say that cleanly #11570

stas00 opened this issue May 3, 2021 · 6 comments · Fixed by #11573

Comments

@stas00
Copy link
Contributor

stas00 commented May 3, 2021

It looks like make fixup et al, require tf (and pt), but fail in unexpected way when the requirements are missing:

python utils/check_repo.py
Checking all models are properly tested.
Checking all objects are properly documented.
Checking all models are in at least one auto class.
Traceback (most recent call last):
  File "/home/michael/projects/transformers/utils/check_repo.py", line 481, in <module>
    check_repo_quality()
  File "/home/michael/projects/transformers/utils/check_repo.py", line 477, in check_repo_quality
    check_all_models_are_auto_configured()
  File "/home/michael/projects/transformers/utils/check_repo.py", line 290, in check_all_models_are_auto_configured
    all_auto_models = get_all_auto_configured_models()
  File "/home/michael/projects/transformers/utils/check_repo.py", line 253, in get_all_auto_configured_models
    for attr_name in dir(transformers.models.auto.modeling_tf_auto):
  File "/home/michael/projects/transformers/src/transformers/file_utils.py", line 1690, in __getattr__
    raise AttributeError(f"module {self.__name__} has no attribute {name}")
AttributeError: module transformers.models.auto has no attribute modeling_tf_auto
make: *** [Makefile:35 : extra_quality_checks] Erreur 1

Thank you, @michaelbenayoun for flagging this

Should we add a small script first that checks all the requirements so that the error is not misleading, but something like: "need to install pip install -e .[dev] to develop transformers"?

@sgugger, @LysandreJik

@stas00 stas00 changed the title [fixup/style] requires TF but doesn't handle that [fixup/style] requires TF but doesn't say that cleanly May 3, 2021
@michaelbenayoun
Copy link
Member

michaelbenayoun commented May 3, 2021

The same thing happens with flax.

@stas00
Copy link
Contributor Author

stas00 commented May 3, 2021

This should take care of all the deps:

pip install -e .[dev]

Please let us know if it didn't.

@sgugger
Copy link
Collaborator

sgugger commented May 3, 2021

I don't think we need a new script for that. Maybe add the check inside the script that fails (check_all_models_are_auto_configured) and issue a warning if not all backends are detected (I don't think we need to error out, since it's unlikely the user will bring changes that break a backend when that backend is not even installed)? I can do this later this evening or tomorrow.

@stas00
Copy link
Contributor Author

stas00 commented May 3, 2021

Also with a bit of some further massaging of setup.py's extras, we could automate this - basically need to be able to load extras[dev] outside of setup.py, so the check could be to simply import everything that is in extras[dev].

@sgugger
Copy link
Collaborator

sgugger commented May 3, 2021

Note that this specific script only relies on the model backends only, so no need for the whole of dev yet.

@stas00
Copy link
Contributor Author

stas00 commented May 3, 2021

If it's easier - then by all means.

I just thought that if we already maintain extras[dev] then any dev tool could just have that as pre-requisite.

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 a pull request may close this issue.

3 participants