YAML configuration for mlx_lm.lora

[chimezie]

Changes lora tuning module to use YAML configuration with defaults from original command-line parameters

Supersede #235

[awni]

Thanks for getting this started! I suggest a few changes (which I'm happy to help with):

1. Keep support for the existing command line arguments. It can be nice to run with just the CLI and it's useful to be back-compatible here IMO.
2. Allow a config arg for the yaml.
3. I think a nice behavior is to overwrite flags from the config with the CLI (so prefer the command line to the same parameter set in the config). Makes it easy to experiment without needing to update the config. Another option which I think is ok is to simply disallow setting both (if the config is provided then you can't also provide the same flag on the CLI).

[awni]

Let's keep the LORA.md as it was. Instead of putting this here, let's put an example config in the examples directory (call it lora_config.yaml). https://github.com/ml-explore/mlx-examples/tree/main/llms/mlx_lm/examples

[awni]

And maybe link to the example config in this readme and mention how to use it.

[chimezie]

Ok. I reverted LORA.md, added the example YAML, and added a mention of it in LORA.md

[awni]

What is the purpose of this?

[chimezie]

See: Numbers in scientific notation without dot are parsed as string

[awni]

@chimezie do you plan to come back to this? Let me know if I can help here!

[chimezie]

Yes, I was working on it a few hours ago and intended to finish it tomorrow (with some questions).

[chimezie]

1. Keep support for the existing command line arguments. It can be nice to run with just the CLI and it's useful to be back-compatible here IMO.

I agree. I have updated this

2. Allow a config arg for the yaml.

There was already a positional argument for the config (it is now an optional positional argument). Is it fine as it is, or did you want it as an optional argument like (for example) -c/--config?

3. I think a nice behavior is to overwrite flags from the config with the CLI (so prefer the command line to the same parameter set in the config). Makes it easy to experiment without needing to update the config. Another option which I think is ok is to simply disallow setting both (if the config is provided then you can't also provide the same flag on the CLI).

Ok. I went with the first option, so the CLI flags take precedence

[awni]

Thank you for all the updates!

There was already a positional argument for the config (it is now an optional positional argument). Is it fine as it is, or did you want it as an optional argument like (for example) -c/--config?

I think it would be better as a flag -c/--config because it is more consistent with all the other arguments

[chimezie]

Ok. I have updated the config option accordingly. I also ended up having to remove the defaults from the CLI argument definitions because they were overriding the YAML-based options due to their taking precedence. Currently, the order of precedence is:

1. CLI options (if specified)
2. YAML options (if specified)
3. Defaults in CONFIG_DEFAULTS

It is ready for your review

[chimezie]

One tricky issue came up afterwards.

Apparently, even if you define options this way:

import argparse

parser = argparse.ArgumentParser(description='')
parser.add_argument('--sum', help='', required=False)
args = parser.parse_args()

if __name__ == '__main__':
    print(args.__dict__)

The argument dict will still have an entry for the 'sum' option, mapped to None. So, as it is currently, if the -c/--config is not provided, all the defaults will be None, even for CLI options that were not specified. What I had in mind was to do this, but I'm reluctant for no particularly good reason than that argparse should exclude options that were not specified in the CLI if they are defined to not be required:

    if args.config:
        # [..]    
    else:
        args.__dict__.update(
            {
                arg: CONFIG_DEFAULTS[arg]
                for arg, value in args.__dict__.items()
                if value is None and arg != "config"
            }
        )

Thoughts?

[madroidmaq]

Can you just set it to adapter.npz to make sure it works out of the box?

[chimezie]

That file is just an example of the format of the YAML configuration and the mlx_lm/lora.py here pulls defaults from CONFIG_DEFAULTS in that file:

CONFIG_DEFAULTS = {
    "adapter_file": "adapters.npz",
   [...]
}

Since 'adapters.npz' is already the default for adapter_file there, if --adapter-file is not specified in the commandline or in the configuration (as "adapter_file") then adapter.npz will be the default.

[awni]

Thanks for the additions!