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

Explicitly list the exported symbols #249

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Explicitly list the exported symbols #249

merged 1 commit into from
Nov 22, 2023

Conversation

dustalov
Copy link
Contributor

Due to the lack of an explicit list of exported symbols in the __all__ variable, a strict mode of mypy and possibly other type checkers emit errors about implicit exports:

error: Module "sacrebleu" does not explicitly export attribute "CHRF"  [attr-defined]

This pull request addresses this problem by exporting all the symbols imported in the top-level __init__.py.

Copy link
Collaborator

@martinpopel martinpopel left a comment

Choose a reason for hiding this comment

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

I think this is OK, but I would appreciate @ozancaglayan or @mjpost to confirm it (and merge this PR).

@dustalov
Copy link
Contributor Author

Could you run the check-build workflow for this PR? I have not modified the check scripts, but did not run the action in my fork repository because I committed the code before I noticed that I had to approve the workflows.

@martinpopel
Copy link
Collaborator

The check-build is OK.
@ozancaglayan and @mjpost: let us know if you have some comments, otherwise I will merge this PR tomorrow. I was not sure if some of the symbols are not internal or legacy, so that their usage should be deprecated (and after the deprecation period, we should remove them from this __init__.py). However, this question is orthogonal to this PR and I agree code using sacrebleu should not result in any mypy --strict errors caused by SacreBLEU.

@dustalov: Please delete also the # noqa: F401 comments. They won't be needed once all the symbols are explicitly exported (and thus used) in __all__.

@dustalov: Do you plan to run mypy also on sacrebleu (or only on your libraries which use sacrebleu)? If yes, we should perhaps also prevent another mypy error (of the same type) by replacing:

from .metrics import BLEU, CHRF, TER

with

from .metrics.bleu import BLEU
from .metrics.chrf import CHRF
from .metrics.ter import TER

Am I right?

@martinpopel martinpopel merged commit 3150f2b into mjpost:master Nov 22, 2023
17 checks passed
@dustalov
Copy link
Contributor Author

Thank you very much for merging this! Sorry for not responding quicker, my company runs an onsite gathering this week, which reduced my screen time dramatically.

I can submit the removal of # noqa: F401 comments in a separate PR, and I used mypy to check types of my code that uses SacreBLEU. My version passes the check well, but I have not tried it for the entire codebase. I think it should be a part of the CI process.

@dustalov dustalov deleted the export branch November 22, 2023 13:46
@martinpopel
Copy link
Collaborator

I understand, my time for SacreBLEU is also very limited. We could have another PR that would fix some/all of the errors/warnings in SacreBLEU reported by flake8 and mypy. In the end I've decided that such PR is not related to this PR, so I should not wait with merging.

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.

2 participants