Refactor factory into separate config parser and unified create_dsp() construction path#227
Refactor factory into separate config parser and unified create_dsp() construction path#227jfsantos wants to merge 4 commits intosdatkinson:mainfrom
Conversation
Add typed config structs (LinearConfig, LSTMConfig, ConvNetConfig, WaveNetConfig) and parse_config_json() functions per architecture. Introduce ModelConfig variant, ModelMetadata, and create_dsp() for unified model construction independent of JSON parsing. Refactor get_dsp() to use the new unified path. Register Linear factory (was previously missing). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sdatkinson
left a comment
sdatkinson
left a comment
There was a problem hiding this comment.
This is pretty slick overall!
The big problem, however, is that it seems to kill the registry pattern that makes this library extensible with new architectures?
Perhaps I owe you a test that compile-time registers a new "test" architecture. Let me do that so that the requirement is more clear.
Otherwise, very cool!
NAM/get_dsp.cpp
Outdated
There was a problem hiding this comment.
[C] It appears that the registry doesn't work anymore with this?
If I have another repo with a new NAM architecture, it won't get picked up anymore.
| { | ||
| std::string version; | ||
| double sample_rate = -1.0; | ||
| std::optional<double> loudness; |
NAM/model_config.h
Outdated
There was a problem hiding this comment.
[C] Can this be added to by code outside of this repo?
NAM/wavenet.cpp
Outdated
There was a problem hiding this comment.
Claude seems to have thrashed the comments, which is making it hard for my eyes to see the changes. Can you revert those?
I think this is correct but
|
Try merging #231 in. If that test still passes, then I think my concerns would be allayed. |
…o feature/config-refactor
ModelConfig is now an abstract class with a virtual create() method instead of a std::variant over built-in config types. Each architecture's config struct inherits from ModelConfig and implements create(). A single ConfigParserRegistry maps architecture names to parser functions that return unique_ptr<ModelConfig>. Both built-in architectures (via ConfigParserHelper) and external ones (via factory::Helper, which now wraps into FactoryConfig) register in the same registry. This can be refactored such that we only have ConfigParserHelper, but I wanted to keep the API backwards-compatible.
|
I updated the machinery to work similarly to how it worked before with a common registry for both internal and external models (the only difference being that the registry is now for parsers and not full-on constructors). Merged with #231 (tests now pass), added missing comments back. Something I just thought could be done is register parsers with a format (e.g. |
2 participants
Add typed config structs (LinearConfig, LSTMConfig, ConvNetConfig, WaveNetConfig) and parse_config_json() functions per architecture. Introduce ModelConfig variant, ModelMetadata, and create_dsp() for unified model construction independent of JSON parsing. Refactor get_dsp() to use the new unified path. Register Linear factory (was previously missing).
This is being done in order to support parsers for formats other than JSON more easily. Those parsers should NOT be a part of Core, at least for now. I will be creating a separate repo for the binary .namb format that leverages this refactor if it gets merged.
Developed with support and sponsorship from TONE3000