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

Adds TQDMProgressBar in callbacks #610

Merged
merged 10 commits into from
Oct 30, 2019
Merged

Adds TQDMProgressBar in callbacks #610

merged 10 commits into from
Oct 30, 2019

Conversation

shun-lin
Copy link
Contributor

Adding TQDMProgressBar in callbacks to make integration of TQDM (for progress bar) with tensorflow easy and configurable.

I also included ipython notebook usage / demo in the docs/tutorial folder.

Original source code:
https://github.com/bstriner/keras-tqdm
https://gist.github.com/ebursztein/5ff21d7600556de9946eac4a313344be

Original Pull Request :
#519
(sorry I messed up with merging and thus I have create another repo, the current one, to make pull request).

Based on the comments on 519:

  1. I have ran bash tools/run_docker.sh -c 'make code-format' and all tests (3) passes.
  2. I have updated callback README.

Thank you so much for your time!

@shun-lin shun-lin requested review from Squadrick and a team as code owners October 21, 2019 07:35
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@shun-lin
Copy link
Contributor Author

@seanpmorgan I have fixed a typo that is causing tests to fail, may you add the kokoro:force-run label again? Thank you so much for your time!

@seanpmorgan
Copy link
Member

@seanpmorgan I have fixed a typo that is causing tests to fail, may you add the kokoro:force-run label again? Thank you so much for your time!

Hmmmm looks like there is still a formatting error that is found. Can you double check that any re-formats were committed/pushed

@shun-lin
Copy link
Contributor Author

@seanpmorgan I have fixed a typo that is causing tests to fail, may you add the kokoro:force-run label again? Thank you so much for your time!

Hmmmm looks like there is still a formatting error that is found. Can you double check that any re-formats were committed/pushed

@seanpmorgan sorry I forgot to check sanity with bash tools/run_docker.sh -c 'make sanity-check' I have ran it and updated the code to pass all the sanity check, may you add kokoro:force-run label again? Thank you so much for your time!

Another question, I found out that when someone just look at the notebook, the private output of the kernal does not show the progress bar, where should I put the below screenshot (or if I should at all)? If it would be helpful, I am thinking of putting it under doc/tutorial/assets and then link it in the notebook. Thank you so much!
https://drive.google.com/file/d/1EEqoF37tknt5Gtl5GCp9_wDh5CRDqF15/view?usp=sharing

@shun-lin
Copy link
Contributor Author

shun-lin commented Oct 24, 2019

@Squadrick, @seanpmorgan when you have time may you review this PR? Thank you so much for your time!

Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Almost ready to merge, please see the comments.

tensorflow_addons/callbacks/README.md Outdated Show resolved Hide resolved
@seanpmorgan
Copy link
Member

Also just want to tag @bstriner. The repo has an MIT license so I think it's fine but wanted to tag for visibility

@shun-lin
Copy link
Contributor Author

Also just want to tag @bstriner. The repo has an MIT license so I think it's fine but wanted to tag for visibility

Thanks Sean!

@shun-lin
Copy link
Contributor Author

@seanpmorgan I have updated this Pull Request based on your comments, may you run kokoro:force-run again and review the PR? Thank you so much for your time!

Copy link
Member

@seanpmorgan seanpmorgan 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 the updates. So I tried running this in colab and didn't return the expected result. Any ideas?
https://colab.research.google.com/drive/1yIRkyVZEbOWtAzr2BKDoyDeCJFJinR7d

tensorflow_addons/callbacks/tqdm_progress_bar.py Outdated Show resolved Hide resolved
@shun-lin
Copy link
Contributor Author

shun-lin commented Oct 30, 2019

Thanks for the updates. So I tried running this in colab and didn't return the expected result. Any ideas?
https://colab.research.google.com/drive/1yIRkyVZEbOWtAzr2BKDoyDeCJFJinR7d

Hi @seanpmorgan, I finally found out why! The tqdm version on that runtime is 4.28.1 instead of the latest 4.36.1. I think somewhere between 4.28.1 and 4.36.1 they fixed the issue, I will update the PR based on your comments and also make sure on colab it is installing version >=4.36.1, thank you so much for pointing this out!

Edit: found the solution!

@shun-lin
Copy link
Contributor Author

@seanpmorgan I have updated this PR based on your comments and found the solution to the issue mentioned (tqdm>=4.36.1 solves it). When you have time may you review it again? Thank you so much for your time!

@shun-lin shun-lin requested a review from seanpmorgan October 30, 2019 15:39
Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the contribution!

@seanpmorgan seanpmorgan merged commit a10898e into tensorflow:master Oct 30, 2019
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.

4 participants