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

[core] implement support for run-time dependency version checking #8645

Merged
merged 24 commits into from
Nov 24, 2020

Conversation

stas00
Copy link
Contributor

@stas00 stas00 commented Nov 19, 2020

As discussed at #8073 (comment) this PR:

  • adds mechanics for run-time dependency version checks (module with fixed and unversioned too)
  • adds thorough tests (this is where it's the easiest to see how things work)
  • creates one source for all dependency versions in setup.py - need to rerun setup.py on its update to re-generated src/transformers/dependency_versions_table.py, which is then used by transformers
  • adds support and deploys python runtime version check
  • deploys runtime checks for versioned modules in setup.py's install_requires (i.e. must modules)
  • switches examples/lightning_base.py to a fatal-on-failure requirement check.
  • deploys the version lookup setup.py's extras definitions and install_requires
  • adds new Makefile target deps_table_update that updates the dep table, and inserts it into style/quality/fixup targets so the sync shouldn't take too long if forgotten to be run explicitly

@sgugger, @LysandreJik, @patrickvonplaten, @thomwolf

setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

This is a great addition and I think will save us from a lot of repetitive version mismatch issues - thanks a lot @stas00 !

Two things:

  • I don't think tokenizers is a mandatory dependency
  • Not sure if need this functionality to write a dependency table file. Curious to hear your thoughts here though!

@stas00
Copy link
Contributor Author

stas00 commented Nov 20, 2020

@sgugger, wrt your comment in your announcement

=> Resulting breaking change: some people will have to install sentencepiece explicitly while they didn’t have to before with the command pip install transformers[sentencepiece].

After this PR, you can just require_version("sentencepiece", "pip install transformers[sentencepiece]") in the code that needs it at run-time. You can expand the hint (second arg) as you wish to be self-explanatory.

setup.py Outdated Show resolved Hide resolved
@stas00 stas00 removed the request for review from thomwolf November 23, 2020 20:30
@stas00
Copy link
Contributor Author

stas00 commented Nov 24, 2020

@LysandreJik and @sgugger - it's ready to merge whenever you have a chance to review. Thank you.

Probably it is best to merge post v4-release, in case I missed something.

@stas00 stas00 changed the title [WIP] [core] implement support for run-time dependency version checking [core] implement support for run-time dependency version checking Nov 24, 2020
@sgugger
Copy link
Collaborator

sgugger commented Nov 24, 2020

Will do a final review tomorrow!

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.

Very nice! Could you please pass the new parts of the setup.py though the formatter? There are some inconsistencies with the rest of the library (using single quotes instead of double quotes, dicts have weird spacing). Thanks!

Makefile Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
src/transformers/utils/versions.py Outdated Show resolved Hide resolved
stas00 and others added 5 commits November 24, 2020 08:20
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

This is great. Thank you for your work on this Stas.

@LysandreJik LysandreJik merged commit 82d443a into huggingface:master Nov 24, 2020
@stas00 stas00 deleted the runtime-dep-check branch November 24, 2020 18:32
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.

4 participants