-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Separate packing and uploading stages #5752
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5752 +/- ##
==========================================
- Coverage 54.27% 52.05% -2.22%
==========================================
Files 767 767
Lines 70338 70092 -246
==========================================
- Hits 38173 36484 -1689
- Misses 32165 33608 +1443
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
- There is a CI issue. I'll rerun it. If it still exists, it might be due to this PR. Let's check it.
- @ftshijt, can you also review this PR (again)?
Hmm, we seem to have some mysterious doc error. |
I don't think so. It seems that the problem is related to notebook, but I don't remember making any changes to I think it's related to https://github.com/espnet/notebook. There is no Line 29 in a83f82b
Line 17 in a83f82b
|
Thanks for your investigation. You're right. |
The notebook error is fixed. I don't know why the following error occurs: I did make changes to |
Thanks for the report. @simpleoier, can you check this error? |
Hi @cromz22, thanks for your changes! |
Thank you! I see, sorry I overlooked that. |
Looks like CI was successful (except for MacOS errors). |
Thanks a lot, @cromz22! |
What?
This pull request includes changes to TEMPLATE for each task separating packing and uploading stages.
Specifically, this includes the following changes:
egs2/TEMPLATE/<task>/<task>.sh
for the above objectivedoc/espnet2_tutorial.md
accordinglyci/test_integration_espnet2.sh
accordinglyNotes on details of implementation
st1
,mt1
, ands2st1
, the packing and uploading to Zenodo stages were tied and would have produced errors when executing the script.diar1
,enh1
,enh_diar1
,slu1
,svs1
,tts1
, anduasr1
,skip_upload
implicitly meant uploading to Zenodo. It will be less confusing for programmers owing to the change of variable names.asr1
,asr2
,lm1
,spk1
, ands2t1
, which stages to skip were determined by additional if statement, therefore they would have worked without this pr. The changes made are for consistency.enh_asr1
,enh_st1
,spk1
, andssl1
, there are no stages of uploading to Zenodo, so this would also have worked without this pr. The changes made are for consistency.asvspoof
, there were no implementation of packing and uploading, so I kept it that way.st1
in this pr to avoid confict when merging with Bug fixes for egs2/TEMPLATE/st1 #5748.Why?
Uploading models to Zenodo is deprecated and uploading them to huggingface is encouraged (cf. CONTRIBUTING.md). In some tasks, however, the packing stage and uploading to Zenodo stage are tied together with
skip_upload
, making it difficult to only upload to huggingface. The changes made here makes it possible to do that by specifying--skip_packing false --skip_upload_hf false
.See also
Issue #5748