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

Extra dependances #596

Merged
merged 14 commits into from
Jun 17, 2024
Merged

Extra dependances #596

merged 14 commits into from
Jun 17, 2024

Conversation

ayasyrev
Copy link
Contributor

  • [* ] I've checked contribution guide.

Add "extra" options for pip install.

  • tests
  • optional
  • loggers
  • nlp

If OK, add info to documentation.
Now we can move "tensorboard" to loggers and create cv requrements file

@ayasyrev
Copy link
Contributor Author

Now "extra" is: "optional", "nlp", "full" and "tests".
Later will be "cv", "pipelines", "audio".

Copy link
Contributor

@AlekseySh AlekseySh left a comment

Choose a reason for hiding this comment

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

I suggest some changes. If we go this way, we will only have "full" and "nlp" pip tags for now. We will add "audio" and "cv" later, but not "optional" and "tests"

Also, let's update installation instruction. Please, check contribution guide to understand how to change something in Readme.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
ci/requirements_optional.txt Show resolved Hide resolved
ci/requirements_tests.txt Outdated Show resolved Hide resolved
@AlekseySh AlekseySh linked an issue Jun 15, 2024 that may be closed by this pull request
@AlekseySh
Copy link
Contributor

@ayasyrev everything looks okay to me

let's finish with my comment above
and fix that numpy error, it should be simple

after that we are good to merge

@DaloroAT
Copy link
Collaborator

Do we need to add typings from transformers for NLP related modules (datasets, models)? Right now placeholders with Any type there. Or we can do it in separate PR.

@AlekseySh
Copy link
Contributor

@DaloroAT

#596 (comment)

I've tried and failed in the previous PRs. Spent a few hours on that. Seems like I could not make work type hints when they created in if-else condition. Probably I can may it work... I dont'know

.github/workflows/tests.yaml Outdated Show resolved Hide resolved
@DaloroAT DaloroAT closed this Jun 17, 2024
@DaloroAT DaloroAT reopened this Jun 17, 2024
@DaloroAT
Copy link
Collaborator

I've tried and failed in the previous PRs. Spent a few hours on that. Seems like I could not make work type hints when they created in if-else condition. Probably I can may it work... I dont'know

Now you can import Tokenizers and models from transformers in codebase without if-else. It makes sense, because lib might be installed with [nlp] extra

@AlekseySh
Copy link
Contributor

Now you can import Tokenizers and models from transformers in codebase without if-else. It makes sense, because lib might be installed with [nlp] extra

@DaloroAT
what if I installed library without nlp? I will get error when imports chain will be executed

setup.py Show resolved Hide resolved
@ayasyrev
Copy link
Contributor Author

Return tensorboard to main requirements. It needs for short tests.

@AlekseySh AlekseySh merged commit 4872972 into OML-Team:main Jun 17, 2024
8 checks passed
@ayasyrev ayasyrev deleted the extra_dependances branch June 18, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Move more requirements to optional
3 participants