[NCF] Add run_eagerly for ctl.#7229
Conversation
guptapriya
left a comment
guptapriya
left a comment
There was a problem hiding this comment.
Thank you for adding this option!
guptapriya
left a comment
guptapriya
left a comment
There was a problem hiding this comment.
Ah actually I got confused about what this PR does. What is your intention with this PR?
Earlier i thought "Default" value of the flag means default strategy. but it doesn't. It means "pick the best strategy based on num gpus etc.. " https://github.com/tensorflow/models/blob/master/official/utils/misc/distribution_utils.py#L93
I am assuming that's not your intention?
|
Fixed and updated the comment on the test. PTAL |
|
nice, this will allow us to run eager with CTL. Just a question, I thought eager mode does not support distribution strategy? |
| FLAGS.early_stopping = True | ||
| self._run_and_report_benchmark() | ||
|
|
||
| def benchmark_1_gpu_ctl_run_eagerly_early_stop(self): |
There was a problem hiding this comment.
Do we also want to do a non dist strat run eagerly?
There was a problem hiding this comment.
Now I am confused again; and now I remember dist strat needs turned off. I always look at this branch last when I am rushing to finish other stuff. One more try coming up. :-)
There was a problem hiding this comment.
Other than removing tests this is the last test I am adding period... probably....no seriously...maybe.
|
hi, the changes look good to me. Should be go ahead and checkin this change? |
4 participants
No description provided.