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

Move texar to extra require #720

Merged
merged 102 commits into from
Apr 12, 2022
Merged

Move texar to extra require #720

merged 102 commits into from
Apr 12, 2022

Conversation

hepengfe
Copy link
Collaborator

@hepengfe hepengfe commented Mar 31, 2022

This PR fixes #719.

Description of changes

  • raise ImportError with suggestions when the extra library texar-pytorch is not installed

Possible influences of this PR.

  • User doesn't need to install pytorch by default for using main texar functions

Test Conducted

  • Implemented texar_non_dependency_test.py that imports classes that doesn't have dependency on texar-pytorch but might trigger importing torch accidentally through __init__.py or importing torch at the head of class files.

@hunterhector
Copy link
Member

@feipenghe @mylibrar Please notice that the solution cannot be simply throwing exceptions at run time.

If we decide Texar would be extra required, we need to provide a good extra requirement install option in setup.py, and in the documentation here: https:/asyml/forte#download-and-installation, explain which ones require which extra requirement.

Note that you also cannot simply say pip install forte[texar], it is not intuitive. It should be related to the function itself. For example, if the users need to use the models (srl, ner), the extra requirement should be pip install forte[forte-models]. If the function is related to data augmentation models, the extra requirement could be pip install forte[data-aug-models].

Here is the goal: the user shouldn't encounter any exception if he follows the documentation. The exceptions are pointers for a user to notice the documentation, not documentations by themselves.

README.md Outdated 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
setup.py Outdated 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
cd texar-pytorch
pip install --progress-bar off .
cd ..
rm -rf texar-pytorch
Copy link
Member

@hunterhector hunterhector Apr 11, 2022

Choose a reason for hiding this comment

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

why don't we run these steps before the Install Texar or even the Install deep learning frameworks step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we still have dependencies on other libraries in the dependency matrix. In this PR, we only need to remove texar. In the later PR that we set up a dependency env for different modules, these lines of code will be removed.

@hepengfe hepengfe merged commit 3c1425a into asyml:master Apr 12, 2022
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.

Move out dependency of texar to extra_requires in forte
3 participants