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

Make stwfsapy an optional dependency #700

Merged
merged 4 commits into from
May 8, 2023
Merged

Make stwfsapy an optional dependency #700

merged 4 commits into from
May 8, 2023

Conversation

cbartz
Copy link
Contributor

@cbartz cbartz commented May 8, 2023

Closes #699

@osma osma added this to the 1.0 milestone May 8, 2023
@osma osma requested a review from juhoinkinen May 8, 2023 07:56
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (3451bd3) 99.63% compared to head (84fa56d) 99.63%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #700   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files          89       89           
  Lines        6222     6231    +9     
=======================================
+ Hits         6199     6208    +9     
  Misses         23       23           
Impacted Files Coverage Δ
annif/backend/__init__.py 100.00% <100.00%> (ø)
tests/test_backend.py 100.00% <100.00%> (ø)
tests/test_backend_stwfsa.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@juhoinkinen juhoinkinen left a comment

Choose a reason for hiding this comment

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

STWFSA should be added to be installed in the Docker image by adding it to the optional_dependencies variable in

ARG optional_dependencies="fasttext voikko fasttext nn omikuji yake spacy"

Otherwise looks good and ready to be merged. The missing test coverage could also be addressed (see the inline comment), but that's a minor thing.

(Annif Wiki needs to be modified to account for the changes, but that should be done when/after next Annif release is made, we should remember that.)

return stwfsa.StwfsaBackend
return stwfsa.StwfsaBackend
except ImportError:
raise ValueError("STWFSA not available, cannot use stwfsa backend")
Copy link
Member

Choose a reason for hiding this comment

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

Test coverage could be extended to these lines by adding a test like

@pytest.mark.skipif(
importlib.util.find_spec("yake") is not None,
reason="test requires that YAKE is NOT installed",
)
def test_get_backend_yake_not_installed():
with pytest.raises(ValueError) as excinfo:
annif.backend.get_backend("yake")
assert "YAKE not available" in str(excinfo.value)

@sonarcloud
Copy link

sonarcloud bot commented May 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@juhoinkinen
Copy link
Member

Thanks @cbartz! Merging this now.

@juhoinkinen juhoinkinen merged commit 98dc7c6 into NatLibFi:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make stwfsapy an optional dependency
3 participants