Skip to content

Conversation

robieta
Copy link
Contributor

@robieta robieta commented Mar 19, 2018

As noted, imagenet can no longer just use the default number of train_epochs. This PR explicitly sets it back to 100, and also fixes a bug related to "-h" being inadvertently caught by tf.app.run().

@robieta robieta requested a review from karmel March 19, 2018 20:55
@robieta robieta requested review from k-w-w and nealwu as code owners March 19, 2018 20:55
@karmel
Copy link
Contributor

karmel commented Mar 19, 2018

(1) The default should be set for Cifar too.
(2) This is a lot of extra boilerplate to add, just for the sake of running a test, especially considering that most people looking at this code will never see the tests at all. I think either having a parse_args function that gets called before tf.run, or better still just have the test create the class arg parser itself and call main, makes more sense. Remember, most users will just want to read once and run, not understand all of our testing and infra.

@robieta
Copy link
Contributor Author

robieta commented Mar 19, 2018

@karmel The more I look at it I'm inclined to just drop tf.app.run() altogether in favor of just:

if __name__ == "__main__":
  main()

It appears to be primarily for arg parsing, so we basically have 2 arg parsers stepping on each other's toes. From what I've read it's not a necessity.

@robieta robieta force-pushed the update_resnet_argparser branch from 6d68468 to 14a9d2c Compare March 20, 2018 02:49
@robieta
Copy link
Contributor Author

robieta commented Mar 20, 2018

Removing tf.app.run significantly simplified the amount of "counter magic" needed.


def main(argv):
def main(argv=None):
argv = sys.argv if argv is None else argv
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this at all if we're calling main directly?

Copy link
Contributor Author

@robieta robieta Mar 20, 2018

Choose a reason for hiding this comment

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

The end to end test passes its own args instead of using sys.argv. I suppose we could also just make argv a required arg. Typically this argv=None pattern is what I've seen.

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 it's more conventional then to have the if name == section pass in sys.argv, rather than having this extra line here.

@robieta
Copy link
Contributor Author

robieta commented Mar 20, 2018

The argv call is now in the ==main section.

…lp interception caused by moving arg parsing into main functions for resnet.

remove tf.app.run from resnet

make argv explicit for main functions
@robieta robieta force-pushed the update_resnet_argparser branch from c29d807 to 94c0177 Compare March 20, 2018 20:14
@robieta robieta merged commit 5ef68f5 into master Mar 20, 2018
@robieta robieta deleted the update_resnet_argparser branch March 20, 2018 20:32
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.

3 participants