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

fix the famous WeakSet keyError bug #607

Merged
merged 20 commits into from Oct 15, 2018
Merged

Conversation

chengs
Copy link
Contributor

@chengs chengs commented Sep 10, 2018

This PR fixes #596 #548 #553, by checking the existence of an instance before removing it from tqdm._instances (WeakSet)
Another solution to fix them is to ignore the KeyError in the following code.

  • fix error in test_perf.

I think I almost find why this problem occurs.
@casperdcl See this
v4.19.1...tqdm:v4.24.0

It is highly possible that, the problem is because of this part in _tqdm.py
screen shot 2018-09-10 at 3 43 36 pm
In v4.19, when removing an instance from WeakSet, the KeyError is ignored.
Magically, now it will raise an error. (well, I don't have time to check why someone changes this to raise an error).

However, we can avoid this keyError.
It seems in python2, when tqdm is called with for like this

for _ in trange(10):
   break

py2 will magically collect the hidden object of trange(10) after break, which causes this keyError.
I say this because this code works well.

pbar = tqdm(range(10))
for i in pbar:
    if i==2:
        break

So, it may happen that the tqdm._instances(WeakSet) does not know the instance is already removed, but it still try to remove the instance and causes the keyError. We can add a check step before the removal.

@chengs chengs changed the title fix the famous WeakSet keyError bug [not ready] fix the famous WeakSet keyError bug Sep 10, 2018
@codecov-io
Copy link

codecov-io commented Sep 10, 2018

Codecov Report

Merging #607 into master will decrease coverage by 0.23%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
- Coverage   98.87%   98.63%   -0.24%     
==========================================
  Files           9       10       +1     
  Lines         709      731      +22     
  Branches      127      130       +3     
==========================================
+ Hits          701      721      +20     
- Misses          5        6       +1     
- Partials        3        4       +1

@chengs chengs changed the title [not ready] fix the famous WeakSet keyError bug fix the famous WeakSet keyError bug Sep 10, 2018
@casperdcl casperdcl self-assigned this Sep 11, 2018
@casperdcl casperdcl added the to-review 🔍 Awaiting final confirmation label Sep 11, 2018
@chengs chengs mentioned this pull request Sep 14, 2018
@jgsogo jgsogo mentioned this pull request Sep 14, 2018
5 tasks
@chengs
Copy link
Contributor Author

chengs commented Sep 14, 2018

v4.19.1...tqdm:v4.24.0
v4.20.0...tqdm:v4.24.0

Until 4.20, it is ok.
It's because of a commit in v4.21. @casperdcl, please check these commits

They changed the behavior of keyError.

@jgsogo
Copy link

jgsogo commented Oct 13, 2018

Hi!

How is this PR going? Some of our users have come across another error when installing our app, Conan, due to the tqdm dependency (here is our open issue: conan-io/conan#3728, here is the one in your repo: #460).

We would want to know if solving this issue will allow us to upgrade the dependency and solve both of them... or maybe we should downgrade tqdm to <4.19.1.

Thanks!

@casperdcl
Copy link
Sponsor Member

#460 is unrelated. I hope to get around to reviewing this PR and all my other backlog soon

@casperdcl
Copy link
Sponsor Member

getting there...

@casperdcl casperdcl added to-merge ↰ Imminent and removed to-review 🔍 Awaiting final confirmation labels Oct 15, 2018
@casperdcl casperdcl merged commit 5aa5924 into tqdm:master Oct 15, 2018
casperdcl added a commit that referenced this pull request Oct 15, 2018
- fix `str.isnumeric` #605
- fix `WeakSet` `KeyError` #548, #553, #596 -> #607
- stop `tqdm_notebook` description truncation #582 -> #599
- include `unit_scale` for `rate` #608
- add `auto` -> nowarning `autonotebook`
- add better postfix numeric formatting #621
- minor refactoring #609 -> #616
- update documentation
- add unit tests
- fix py26 CI
casperdcl added a commit that referenced this pull request Oct 15, 2018
- fix `str.isnumeric` #605
- fix `WeakSet` `KeyError` #548, #553, #596 -> #607
- stop `tqdm_notebook` description truncation #582 -> #599
- include `unit_scale` for `rate` #608
- add `auto` -> nowarning `autonotebook`
- add better postfix numeric formatting #621
- minor refactoring #609 -> #616
- update documentation
- add unit tests
- fix py26 CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception printed when breaking out of loop
6 participants