Make convert_time as State method #2431
Conversation
| if self.dataloader_len is None: | ||
| raise RuntimeError('Cannot convert time, as state.dataloader_len is None.') | ||
| return Time( | ||
| int(time.value * int(self.dataloader_len) * self.max_duration.value), |
There was a problem hiding this comment.
@mvpatel2000, I am not sure if we should apply ssr here. I decided to keep the original implementation but I do not see any reason to apply this scaling once the TimeUnit has changed to BATCH. Similarly to the elif case.
There was a problem hiding this comment.
Hm... i think we probably shouldn't apply SSR here.
Do you mind if I directly touch your PR? There's a few other places we use a similar function (eg runtime estimator) and I think it could be good to consolidate into this API.
Also, apologies for the delay in review. We're in the middle of a release so we're freezing everything but bug fixes. I'll make sure this gets into the next release (0.16.1) which will come very soon after 0.16 (likely a weekish?)
There was a problem hiding this comment.
No worries, feel free to edit the PR 😄
Thanks for your efforts. I am happy to keep contributing with PRs or Issues if I find something.
Let me know if I can help.
There was a problem hiding this comment.
@priba apologies for the delay, this might be on the backburner for a few weeks. Really slammed with other things. If this is a blocker, please let me know and we can try landing a smaller solution. However, I'd really like to get to the full solution and make this much easier for everyone.
There was a problem hiding this comment.
No problem, we already have a workaround. Although it would be a nice feature, I understand it is not a high priority :)
There was a problem hiding this comment.
Hi @mvpatel2000 , are there any news regarding this PR?
There was a problem hiding this comment.
Sorry, I wasn't able to get to it yet. I will make sure to prioritize over next 2-3 weeks!
There was a problem hiding this comment.
@priba sorry for delay, I still haven't gotten to it. I will make sure to do in January though. I keep getting pre-empted with higher priority items
What does this PR do?
This PR exposes
_convert_timeinto the API. As discussed in the related issue, it has been converted into a State method.What issue(s) does this change relate to?
Addresses issue #2426
Before submitting
pre-commiton your change? (see thepre-commitsection of prerequisites)