Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

empty list cannot be cleared issue fixed. #14882

Merged
merged 2 commits into from
Sep 9, 2020
Merged

empty list cannot be cleared issue fixed. #14882

merged 2 commits into from
Sep 9, 2020

Conversation

hsali
Copy link

@hsali hsali commented May 5, 2019

Description

checking list before resetting it.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • check list and clear

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@hsali hsali requested a review from szha as a code owner May 5, 2019 19:44
@anirudhacharya
Copy link
Member

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label May 6, 2019
Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

This change seems to be specific to a python version. (e.g. Python 3.7 allows clearing an empty list. Python 2.7 doesn't have clear().) What version of python did you use?

@@ -141,4 +141,5 @@ def reset(self):
print("Queue size on reset: {}".format(qsize))
for i, p in enumerate(self.proc):
p.join()
self.proc.clear()
if len(self.proc) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a more Pythonic way would be if self.proc ?

Copy link
Author

@hsali hsali May 22, 2019

Choose a reason for hiding this comment

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

I was using python2.7. I faced issue at that time. So I fixed it.

@karan6181
Copy link
Contributor

@hsali Could you please address the review comments? Thanks!

@piyushghai
Copy link
Contributor

@hsali Gentle ping....

@vandanavk
Copy link
Contributor

LGTM.

@karan6181
Copy link
Contributor

@hsali Thanks for your contribution. Could you please address the review comments and Is this PR good to go for merge?

@piyushghai
Copy link
Contributor

Can a committer merge this PR ? This is good to go!

@szha szha changed the base branch from master to v1.x September 7, 2020 07:36
@szha szha self-assigned this Sep 7, 2020
@szha
Copy link
Member

szha commented Sep 7, 2020

@mxnet-bot run ci [all]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [website, windows-cpu, edge, clang, centos-gpu, windows-gpu, unix-cpu, unix-gpu, centos-cpu, sanity, miscellaneous]

@szha szha merged commit a9bd1d2 into apache:v1.x Sep 9, 2020
@szha
Copy link
Member

szha commented Sep 9, 2020

sorry that it took (quite) a while. thanks for the fix @hsali

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants