Refactor hybrid layers to use per-layer configs - #6313
Conversation
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
|
/claude strict-review |
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
|
/ok to test 48cd231 |
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
|
/ok to test ce13069 |
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
|
/ok to test 0a78f80 |
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
|
/ok to test 67c5665 |
There was a problem hiding this comment.
One general comment: I often see type(obj) is LayerConfig instead of isinstance(obj, LayerConfig). The latter seems more canonical to me; is there a reason to prefer the former? (Note, I'm totally fine with keeping the type calls, just feels slightly less pythonic.)
Ah it's to explicitly avoid child classes also matching, isn't it? Could that bite us later?
| affected a shared config, without coupling independently supplied config lists. | ||
| """ | ||
|
|
||
| synchronize_shared_config_mutations = True |
There was a problem hiding this comment.
This attribute is only queried once, but the code never sets it to False. Do we need the _HybridLayerConfigList abstraction class surrounding it? In general I like the idea of having a type alias, but it's also not used for this.
| layer_config = deepcopy(config) | ||
| layer_config.__class__ = LAYER_SYMBOL_TO_CONFIG_CLASS[layer_symbol] |
There was a problem hiding this comment.
I personally find this too hacky and would prefer a cleaner solution, e.g., a separate function that creates the class and then populates the attributes from the deep-copied config.
janEbert
left a comment
janEbert
left a comment
There was a problem hiding this comment.
Approved since I find the design is solid, but please take a look at the comments. :)
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
2 participants
What does this PR do?
See Part 2 of design doc.
Previous PRs: #6410, #6419, #6423.
This PR changes when and how layer configs are constructed. Instead of duplicating the same
TransformerConfigfor each layer (no matter the layer type), now each layer has its own config. Even after this PR, each layer's config will be identical. Each layer config just inheritsTransformerConfig; however, this enables two features:TransformerConfigHere is a simplified view of the new training workflow construction: