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

Show tatoeba process #112

Merged
merged 1 commit into from Apr 1, 2020
Merged

Show tatoeba process #112

merged 1 commit into from Apr 1, 2020

Conversation

DanBmh
Copy link
Contributor

@DanBmh DanBmh commented Mar 29, 2020

I also changed the label type, because this solved a problem i had and the other datasets seemed to have LL_WORD_TRANSCRIPT as default and LL_WORD_TRANSCRIPT_RAW only optionally.

Copy link
Collaborator

@aahlenst aahlenst left a comment

Choose a reason for hiding this comment

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

I understand the desire for progress information, but this does not look like the right approach to me. print() prints to stdout without given users any control over it. Using Python's logging facility instead of print() would help. Then, Audiomate is primarily a library. Using tqdm deep inside the library is not appropriate in my opinion because it forces tqdm upon everyone. Maybe there's a way to offer some sort of callbacks to make using tqdm an opt-in feature.

Regarding LL_WORD_TRANSCRIPT, please open another pull request (separate discussion) and add a test if possible

@DanBmh
Copy link
Contributor Author

DanBmh commented Mar 29, 2020

Ok, i will move the LL_WORD_TRANSCRIPT commit to another branch. What about the updated comment, shall i make a third pull request for it?

Regarding the tqdm and print suggestions, you are using both in the other files, for example here, so i thought it would be ok to use them too.

@aahlenst
Copy link
Collaborator

In my opinion, you can do the comment together with LL_WORD_TRANSCRIPT.

Regarding tqdm/printing, I'm sorry that it hit you. Let's see what @ynop has to say.

@ynop
Copy link
Owner

ynop commented Mar 29, 2020

I added those things when I needed it.
But, @aahlenst is right, it is not a good solution.
I guess we should find a better way to do it.

@DanBmh
Copy link
Contributor Author

DanBmh commented Mar 29, 2020

I would suggest then we merge this with tqdm and print and you find a better way some other time:)

@aahlenst
Copy link
Collaborator

aahlenst commented Apr 1, 2020

@DanBmh #114 brought the necessary infrastructure for logging. Care to update your PR?

@DanBmh
Copy link
Contributor Author

DanBmh commented Apr 1, 2020

Do you plan to implement a prediction for the time to completion in your logger?
I found this helpful in very long running tasks (>4h).

@ynop
Copy link
Owner

ynop commented Apr 1, 2020

No plans so far.
But would be easy to implement based on the 5 minute logging cycle and nice to have.
If you'd like to do it, you can add it and open a PR.

@ynop
Copy link
Owner

ynop commented Apr 1, 2020

Could you merge the two commits? Afterwards I can merge it.

Replace double quotes.

Update to new logging infrastructure.
@DanBmh
Copy link
Contributor Author

DanBmh commented Apr 1, 2020

Done. Why didn't you merge this before updating the logging?

@ynop
Copy link
Owner

ynop commented Apr 1, 2020

Because, I didn't think about that before merging the logging.
And it is cleaner instead of introducing more "bad examples" and correct it right again.

@ynop ynop merged commit 0293548 into ynop:master Apr 1, 2020
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.

None yet

3 participants