-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Deprecate agg_key_funcs
, agg_default_func
, and update_agg_funcs
from LightningLoggerBase
#11871
Conversation
Code looks good. For the deprecation message, can we add more information on what alternatives users could use? |
Co-authored-by: Danielle Pintz <[email protected]>
Can you also deprecate these in this PR, or were you planning to do this in another PR? For these methods, you can put the dep. message in the docstring, but no need to send out a dep warning |
@edward-io Since the aggregation logic is in the training loop now, I'm not sure how (or if) a user can provide custom aggregation functions. Do you know what alternatives there might be? |
I think it makes sense to include in this PR #11832 |
Mentioning this in the deprecation message would be sufficient IMO, but this is just a nit. :) |
Since the aggregation logic is currently a no-op, I don't think it's necessary to mention any alternatives to users, since we are not really removing any functionality. I suppose we could mention the fact that the agg logic is a no-op, but not sure if we want to say that to users. |
Head branch was pushed to by a user without write access
What does this PR do?
Deprecates
agg_key_funcs
,agg_default_func
andupdate_agg_funcs
fromLightningLoggerBase
.Part of #11235
Part of #9145
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃