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

deps: relax version constraint for konoha #3394

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

himkt
Copy link
Contributor

@himkt himkt commented Jan 13, 2024

This PR updates the version constraint for konoha.

I'm so sorry that I didn't notice the issue #3059.
I updated konoha that removed the dependency for importlib-metadata, let me update requirements-dev.txt.

Now konoha only depends on requests with the support for Python3.8+ (https://github.com/himkt/konoha/blob/v5.5.2/pyproject.toml#L27).
So I think it can be safely used with flair and I update the requirements file.

And I have one question for dependency of janome. janome is only used in JapaneseTokenizer, which depends on konoha, therefore janome should be installed in the same place of konoha, I think. There are multiple options:

  • (1) move janome to requirements-dev.txt
  • (2) remove janome from requirements.txt and install with konoha[janome] (extras)
  • (3) move konoha to requirements.txt
  • (4) remove konoha from requirements-dev.txt and install konoha with konoha[janome] (extras)

I'd like to a maintainer's opinion.

@alanakbik alanakbik merged commit d4332d2 into flairNLP:master Feb 2, 2024
1 check passed
@alanakbik
Copy link
Collaborator

@himkt thanks for updating this!

@himkt himkt deleted the konoha-update branch February 2, 2024 14:15
@himkt
Copy link
Contributor Author

himkt commented Feb 2, 2024

And I have one question for dependency of janome.
janome is only used in JapaneseTokenizer, which depends on konoha,
therefore janome should be installed in the same place of konoha, I think.
There are multiple options:

(1) move janome to requirements-dev.txt
(2) remove janome from requirements.txt and install with konoha[janome] (extras)
(3) move konoha to requirements.txt
(4) remove konoha from requirements-dev.txt and install konoha with konoha[janome] (extras)

Thank you @alanakbik! What do you think about that?

@helpmefindaname
Copy link
Collaborator

Hi @himkt
For me the ideal would be to not have neither listed as dependency, as less unused dependencies are better.
However with the current implementation, janome is always required as it will be imported, while konoha is only needed if the JapaneseTokenizer is used. Hence we don't depend on konoha and handle it as an optional dependency instead.

If you have time to refactor the JapaneseTokenizer to use lazy imports on janome, PR's are welcome

@himkt
Copy link
Contributor Author

himkt commented Feb 9, 2024

Thank you @helpmefindaname, it makes sense! I'll work on that later. 👍

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.

3 participants