-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add Training Type Plugins Registry #6982
Add Training Type Plugins Registry #6982
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6982 +/- ##
========================================
- Coverage 92% 81% -11%
========================================
Files 194 196 +2
Lines 12346 14134 +1788
========================================
+ Hits 11331 11407 +76
- Misses 1015 2727 +1712 |
class _TrainingTypePluginsRegistry(UserDict): | ||
""" | ||
This class is a Registry that stores information about the Training Type Plugins. | ||
|
||
The Plugins are mapped to strings. These strings are names that idenitify | ||
a plugin, eg., "deepspeed". It also returns Optional description and | ||
parameters to initialize the Plugin, which were defined durng the | ||
registeration. | ||
|
||
The motivation for having a TrainingTypePluginRegistry is to make it convenient | ||
for the Users to try different Plugins by passing just strings | ||
to the plugins flag to the Trainer. | ||
|
||
Example:: | ||
|
||
@TrainingTypePluginsRegistry.register("lightning", description="Super fast", a=1, b=True) | ||
class LightningPlugin: | ||
def __init__(self, a, b): | ||
... | ||
|
||
""" | ||
|
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.
this opens up a new surface area for backwards compatibility, so we should have clear ways to mark registry entries as deprecated
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.
so we should have clear ways to mark registry entries as deprecated
@ananthsub What do you mean by this?
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.
@kaushikb11 we'll need some annotation as to whether a specific plugin configuration is deprecated. For example, if we want to remove a plugin or a specific configuration, we should give users enough heads up and apply our normal rules for deprecating artifacts.
That means we'll need more metadata associated with the registry entry, such as the description, but also the status of the entry. e.g. if its active or deprecated. this way, when we're reading the entries or instantiating items from the registry, we can warn users if they're using a deprecated plugin
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.
@ananthsub Not sure about this. I think we can only list the active ones here and still handle the deprecation within the Trainer/AcceleratorConnector as we do now. If a plugin is deprecated, IMO it should no longer be listed here, but still supported by the Trainer for the deprecation phase
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.
for deprecating configurations why not:
@TrainingTypePluginsRegistry.register("myplugin", param=1, param_two=True, deprecate_version=1.5)
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.
Great work @kaushikb11!
Co-authored-by: Sean Naren <[email protected]>
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.
Love it ! Great work !
Co-authored-by: Sean Naren <[email protected]>
Co-authored-by: Sean Naren <[email protected]>
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.
I like it!
from pytorch_lightning.utilities.exceptions import MisconfigurationException | ||
|
||
|
||
class _TrainingTypePluginsRegistry(UserDict): |
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.
should we maybe make this a general registry and reuse this with flash? We also have a registry in flash and duplicating this does not make sense...
I think you only have to change the naming of the fields...
cc @tchaton
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.
I did think of a generic LightningRegistry
that could be used for different types. But for now, there has only been a need for TrainingTypePlugins
, we could change it down the road as well, as _TrainingTypePluginsRegistry
is internal.
What does this PR do?
Add Training Type Plugins Registry #6923
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
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 🙃