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

Adding checks for broken bottleneck files #7131

Merged
merged 5 commits into from Feb 3, 2017
Merged

Conversation

rizasif
Copy link
Contributor

@rizasif rizasif commented Jan 29, 2017

An exception is raised when a broken cached bottleneck files is read. Such a file can be created as a result of sudden system failure.

This commit refers the issue #2296 (#2296). Although closed but the problem still persists. Please view comments by @rizasif92 at the end. The changes contain nothing but error handling.

An exception is raised when a broken cached bottleneck files is read. Such a file can be created as a result of sudden system failure. This commit refers the issue tensorflow#2296
@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@rizasif
Copy link
Contributor Author

rizasif commented Jan 29, 2017

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@rmlarsen rmlarsen added the awaiting review Pull request awaiting review label Jan 30, 2017
Copy link
Contributor

@petewarden petewarden left a comment

Choose a reason for hiding this comment

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

Thanks @rizasif , I really appreciate you putting this fix together! Could I request a slightly different approach though? I would prefer to keep the get_or_create_bottleneck() function always returning a bottleneck.

To ensure that, we could move the section nested inside the if not os.path.exists(bottleneck_path): condition into a new function, create_bottleneck_file. Then we can call it from that condition, but also have a flag that catches float exceptions when reading and calls it in that case too. Here's some untested code to try and show what I mean:

  if not os.path.exists(bottleneck_path):
    create_bottleneck_file(...)
  did_hit_error = False
  try:
    bottleneck_values = [float(x) for x in bottleneck_string.split(',')]
  except:
    print("Invalid float found, recreating bottleneck")
    did_hit_error = True
  if did_hit_error:
    create_bottleneck_file(...)
    # Allow exceptions to propagate here, since they shouldn't happen after a fresh creation
    bottleneck_values = [float(x) for x in bottleneck_string.split(',')]

I'm hoping with this approach none of the other changes outside this function should be needed since the contract to always return a list of bottleneck values is preserved.

Does that make sense?

@rmlarsen rmlarsen added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Jan 30, 2017
The patch detects the broken bottleneck files (might be created as a result of sudden system shut down) and recreates them if required.
@rizasif
Copy link
Contributor Author

rizasif commented Jan 31, 2017

@petewarden I agree with your solution and was thinking to implement it. I have made the changes as suggested, and tested them as well; I am attaching a screenshot of the output. Kindly review and let me know if anything else is required.
Thank you :)

capture

@rmlarsen rmlarsen added stat:awaiting tensorflower Status - Awaiting response from tensorflower and removed stat:awaiting response Status - Awaiting response from author labels Jan 31, 2017
Copy link
Contributor

@petewarden petewarden left a comment

Choose a reason for hiding this comment

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

This looking great, thanks for the updates! I have a couple of minor comments, but once those are addressed this looks good to go in.

@@ -481,7 +498,6 @@ def get_random_cached_bottlenecks(sess, image_lists, how_many, category,
ground_truth[label_index] = 1.0
bottlenecks.append(bottleneck)
ground_truths.append(ground_truth)
filenames.append(image_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should put this line back?

@@ -493,11 +509,12 @@ def get_random_cached_bottlenecks(sess, image_lists, how_many, category,
image_index, image_dir, category,
bottleneck_dir, jpeg_data_tensor,
bottleneck_tensor)
ground_truth = np.zeros(class_count, dtype=np.float32)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we revert this change, since now the bottlenecks will never be None?

Reverting some part of code for bottleneck handling changes
@rizasif
Copy link
Contributor Author

rizasif commented Feb 1, 2017

@petewarden done

@rmlarsen
Copy link
Member

rmlarsen commented Feb 1, 2017

@tensorflow-jenkins test this please

@rmlarsen
Copy link
Member

rmlarsen commented Feb 1, 2017

@rizasif it looks like you added some 4 character indents, which breaks the sanity check. TF uses 2 character indents. Please fix.

See: https://ci.tensorflow.org/job/tensorflow-pull-requests-sanity/2923/console

@rmlarsen rmlarsen added stat:awaiting response Status - Awaiting response from author and removed stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Feb 1, 2017
This commit refers to the issue tensorflow#2296
@rizasif
Copy link
Contributor Author

rizasif commented Feb 1, 2017

Indentation changes made, kindly re-run the tests. Thanks :)

@rmlarsen
Copy link
Member

rmlarsen commented Feb 2, 2017

@tensorflow-jenkins test this please

@rmlarsen rmlarsen added awaiting review Pull request awaiting review and removed stat:awaiting response Status - Awaiting response from author labels Feb 2, 2017
@rmlarsen
Copy link
Member

rmlarsen commented Feb 2, 2017

Thanks @rizasif. @petewarden can you take another look? If the logics right to you, approve and I'll get it merged.

Copy link
Contributor

@petewarden petewarden left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@rizasif
Copy link
Contributor Author

rizasif commented Feb 2, 2017

Did the tests pass? :/

@rmlarsen
Copy link
Member

rmlarsen commented Feb 2, 2017

@rizasif it looks like you still have some bad indents, e.g.

FAIL: Found 1 non-whitelited pylint errors:
tensorflow/examples/image_retraining/retrain.py:353: [E0001(syntax-error), ] unexpected indent

@rizasif
Copy link
Contributor Author

rizasif commented Feb 2, 2017

@rmlarsen the code seems to be working at my end; so we might have to do this a few more times to resolve these indentation issues. I hope this commit works fine. @tensorflow-jenkins please test :)

@rmlarsen
Copy link
Member

rmlarsen commented Feb 3, 2017

@rizasif Thanks :)

@tensorflow-jenkins test this please

@rmlarsen rmlarsen merged commit 4a80e9f into tensorflow:master Feb 3, 2017
@rmlarsen
Copy link
Member

rmlarsen commented Feb 3, 2017

@rizasif thanks for the contribution!

@rizasif
Copy link
Contributor Author

rizasif commented Feb 3, 2017

Pleasure's all mine @rmlarsen
Thank you so much for all the support. Hope to see you guys again. Take Care :)

alberto7088 pushed a commit to synthesisai/tensorflow that referenced this pull request Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review Pull request awaiting review cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants