sdatkinson / neural-amp-modeler

Neural network emulator for guitar amplifiers.
MIT License
1.72k stars 130 forks source link

[FEATURE] Extensions #440

Closed sdatkinson closed 4 days ago

sdatkinson commented 2 weeks ago

Background and high-level solution

Is your feature request related to a problem? Please describe. A lot of other developers have special use cases for this repo. They have good ideas for things that would e.g.. simplify their workflow, enable options or defaults that they prefer, etc. Some examples:

The sticking point with a lot of these is that I think of them as "customizations" that not all users will be interested in. Additionally, they add to the complexity of this codebase, and I don't want to be on the hook for maintaining all of them.

Describe the solution you'd like I'm thinking to implement the ability to extend the standardized trainers via the use of plug-ins (but how about let's call them extensions for obvious reasons πŸ˜‰).

Describe alternatives you've considered Currently, folks have two options:

  1. Make a feature request and convince me to take it up. This is tough because this positions me as the maintainer of this repo as a bottleneck. I have limited bandwidth to work on features, approve PRs, and, frankly, sometimes have differing opinions about what should be in this repo. I don't think that many of those ideas are bad; they're just not right for me and this repo of mine.
  2. Fork the repo. Great for me (no responsibility!) but poor for other developers: they may need to make changes that are at odds with changes that I want to make. When I push changes or release a new version, they have the option to either ignore it or attempt to pull in the changes and deal with any conflicts. Since I'm unaware of their use case, this is dangerous for those developers and those conflicts could be pretty substantial.

This solution makes life better for both of us by clarifying what kinds of modifications might be made to this repo: I can guarantee what interfaces will exist for this repo, and others will have the peace of mind knowing that their modifying something that I've (in the abstract) committed to. This should reduce how much conflict resolution is necessary for those writing customizations.

Prototype

First, I modify the GUI trainer to add the following lines immediately below https://github.com/sdatkinson/neural-amp-modeler/blob/66c052b3e57169dc0e107e069f00cf987467db40/nam/train/gui/__init__.py#L26:

def _apply_extensions():
    import importlib
    import os
    import sys

    extensions_path = os.path.join(
        os.environ["HOME"], ".neural-amp-modeler", "extensions"
    )
    if not os.path.exists(extensions_path):
        return
    if not os.path.isdir(extensions_path):
        print(
            f"WARNING: non-directory object found at expected extensions path {extensions_path}; skip"
        )
    print("Applying extensions...")
    if extensions_path not in sys.path:
        sys.path.append(extensions_path)
        extensions_path_not_in_sys_path = True
    else:
        extensions_path_not_in_sys_path = False
    for name in os.listdir(extensions_path):
        if name == "__pycache__":
            continue
        try:
            importlib.import_module(name.removesuffix(".py"))  # Runs it
            print(f"  {name} [SUCCESS]")
        except Exception as e:
            print(f"  {name} [FAILED]")
            print(e)
    if extensions_path_not_in_sys_path:
        for i, p in enumerate(sys.path):
            if p == extensions_path:
                sys.path = sys.path[:i] + sys.path[i + 1 :]
                break
        else:
            raise RuntimeError("Failed to remove extensions path from sys.path?")
    print("Done!")

_apply_extensions()

I've stipulated an extension folder at ~/.neural-amp-modeler/extensions/. Say it contains a file, my_extension.py:

print("Hello extension!")

from nam.train import core
core._CAB_MRSTFT_PRE_EMPH_WEIGHT = 2.0e-3  # Used to be 2.0e-4

Running the GUI trainer then results in the following:

$ nam
Hello extension!

And I've modified the weight for the pre-emphasized MRSTFT loss that's used in cab-fitting (e.g. if some pesky gear doesn't work well with the default).

One bad thing in this example is that I've modified a private attribute of nam.train.core. An author of this extension should feel uneasy about doing this--I might get rid of that attribute without any warning because it's private and I've assumed that it is only responsible for things within the scope of core. The solution to this is to PR to make this attribute public (i.e. rename from _CAB_MRSTFT_PRE_EMPH_WEIGHT to CAB_MRSTFT_PRE_EMPH_WEIGHT.). This is easy to agree to in general (though I might want to do a little cleaning up)--I'm not opposed to these being accessible, but I may be more opinionated about what their values might be for the most general user (which you might not be--and that's ok!) (Or, you can take matters into your own hands and ship the extension as above and 🀞🏻 that things stay working! That's fine with me--it's your responsibility, not mine πŸ˜„)

Details

I'm interested to see how this goes and will make changes based on feedback.