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

Use more linters, better #250

Merged
merged 10 commits into from
Nov 28, 2023
Merged

Use more linters, better #250

merged 10 commits into from
Nov 28, 2023

Conversation

dustalov
Copy link
Contributor

This is a follow-up for #249 that streamlines the build process, fixes type checking, and addresses linting issues.

@dustalov
Copy link
Contributor Author

@martinpopel @mjpost please have a look.

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.

Thanks for fixing these issues.

@dustalov
Copy link
Contributor Author

There was a network error that led to a failed build after my last commit. Could you please restart it?

@martinpopel
Copy link
Collaborator

I've restarted it and one of the checks fails with "error: invalid command 'bdist_wheel'"

@dustalov
Copy link
Contributor Author

I guess it was caused by the missing package wheel, but the tests failed again due to the network errors. Could you restart them, please?

@dustalov
Copy link
Contributor Author

It seems happened again. Could you please restart the tests?

@dustalov
Copy link
Contributor Author

@martinpopel kind reminder

@martinpopel
Copy link
Collaborator

This is at least the second time when the check-build (macos-latest, 3.11) test fails with the same error:

sacreBLEU: Downloading http://data.statmt.org/wmt19/translation-task/test.tgz to /Users/runner/work/sacrebleu/sacrebleu/.sacrebleu/wmt19/raw/wmt19.test.tgz
sacreBLEU: Extracting /Users/runner/work/sacrebleu/sacrebleu/.sacrebleu/wmt19/raw/wmt19.test.tgz to /Users/runner/work/sacrebleu/sacrebleu/.sacrebleu/wmt19/raw
sacreBLEU: System and reference streams have different lengths.

This is just a warning, but the followup test fails because obtained is an empty string not matching the expected output, which is surely a result of the corrupted wmt19 test set. This test on other platforms is OK. I have no idea why this happens and what should we try except for omitting (macos-latest, 3.11) from the tests (other tests are cancelled once the first platform fails).
Maybe changing the http to https? http://data.statmt.org/wmt19/translation-task/test.tgz returns HTTP 301 Moved Permanently
Location: https://data.statmt.org/wmt19/translation-task/test.tgz

I don't think it makes any sense to rerun the tests again.

@dustalov
Copy link
Contributor Author

The build fails on one specific example that works well on my machine (macOS 14.0, Python 3.10.13).

Testing python3 -m sacrebleu -t wmt18,wmt19 -l en-de --echo=src | python3 -m sacrebleu -t wmt18,wmt19 -l en-de -b --detail
sacreBLEU: Downloading http://data.statmt.org/wmt18/translation-task/test.tgz to /Users/<REDACTED>/sacrebleu/.sacrebleu/wmt18/raw/wmt18.test.tgz
sacreBLEU: Extracting /Users/<REDACTED>/sacrebleu/.sacrebleu/wmt18/raw/wmt18.test.tgz to /Users/<REDACTED>/sacrebleu/.sacrebleu/wmt18/raw
sacreBLEU: Downloading http://data.statmt.org/wmt19/translation-task/test.tgz to /Users/<REDACTED>/sacrebleu/.sacrebleu/wmt19/raw/wmt19.test.tgz
sacreBLEU: Extracting /Users/<REDACTED>/sacrebleu/.sacrebleu/wmt19/raw/wmt19.test.tgz to /Users/<REDACTED>/sacrebleu/.sacrebleu/wmt19/raw
PASS

Let's change it to HTTPS and add two more tests triggers: for push and for workflow dispatch.

@dustalov
Copy link
Contributor Author

Let's see if it works with the code that is already in master. I submitted another PR, #252, that simplifies the troubleshooting.

@dustalov
Copy link
Contributor Author

This run was successful just because we were lucky. As long as we rely on a third party that serves the data, we cannot be sure it would work 100% of the time. Now we can merge this PR.

@martinpopel martinpopel merged commit 9e57a51 into mjpost:master Nov 28, 2023
17 checks passed
@martinpopel
Copy link
Collaborator

Thanks for all the work.

@dustalov
Copy link
Contributor Author

Thank you for your patience! Would you mind rolling out a new release so I can update the dependency string in my code?

@mjpost
Copy link
Owner

mjpost commented Nov 28, 2023

Thanks for this, and sorry to be absent from the conversation.

I can do a new release. Would 2.3.3 be appropriate?

Can I ask you to create the release PR, and also to update the CHANGELOG to summarize the changes you've made?

@dustalov
Copy link
Contributor Author

Yes.

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