-
Notifications
You must be signed in to change notification settings - Fork 418
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
Removal of metrics deepcopy before computing the metrics #3180
Removal of metrics deepcopy before computing the metrics #3180
Conversation
Hi @mvpatel2000 should I assign this MR to a maintainer of the repo? Please tell me who I should assign, thank you. |
@gregjauvion feel free to request from me. I can triage if necessary |
@gregjauvion thanks for the PR, I think this is correct. I will run a series of manual tests though to verify identical behavior |
Thanks a lot for your reactivity! Please tell me if I can help in any way. |
Test failures seem related to a flakey test introduced in previous PR, patching elsewhere |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* remove deepcopy of metrics before calling metric.compute() * update documentation of _compute_and_log_metrics --------- Co-authored-by: Grégoire Jauvion <[email protected]> Co-authored-by: Mihir Patel <[email protected]>
* remove deepcopy of metrics before calling metric.compute() * update documentation of _compute_and_log_metrics --------- Co-authored-by: Grégoire Jauvion <[email protected]> Co-authored-by: Mihir Patel <[email protected]>
What does this PR do?
This PR removes the deep copy of the
metrics: Dict[str, Metric]
object in the function_compute_and_log_metrics
.This function does the following:
copy.deepcopy
)It is called 3 times in
composer.trainer.Trainer
:I don't see the need for copying the metrics before calling metric.compute(). In particular, the metrics are reset with metric.reset() at the start of the training on each batch, and at the start of the evaluation loop, making this copy useless. Is there a specific reason I am missing?
What issue(s) does this change relate to?
Related to issue #3153 describing that copying the metrics results in a memory leak in a specific use-case I'm working on.
Before submitting
pre-commit
on your change? (see thepre-commit
section of prerequisites)