LLamaSharp icon indicating copy to clipboard operation
LLamaSharp copied to clipboard

Avoid declaring constructors with parameters if the properties of the type can be obtained from configuration settings.

Open vvdb-architecture opened this issue 2 years ago • 2 comments

When upgrading to the latest version of the LLama source, I can no longer get the LlamaSharpConfig from the configuration setting like so:


            var llamaSharpConfig = GetConfigSettings<LLamaSharpConfigExt>(configuration, "LlamaSharpConfig");

        private static T GetConfigSettings<T>(IConfiguration configuration, string key) where T : class
        {
            var value = (configuration.GetSection(key)?.Get<T>()) ?? throw new ArgumentNullException(nameof(configuration), $"Can't find {key} section of type {typeof(T).Name} in configuration");
            return value;
        }

... because of the nondefault constructor that was introduced in LlamaSharpConfig:

        /// <summary>
        /// Initializes a new instance of the <see cref="LLamaSharpConfig"/> class.
        /// </summary>
        /// <param name="modelPath">The path to the model file.</param>
        public LLamaSharpConfig(string modelPath)
        {
            ModelPath = modelPath;
        }

The workaround is simple: just derive from it:

        /// <summary>
        /// We need this workaround because of nondefault constructor in <see cref="LLamaSharpConfig"/> doesn't allow this type to be read from an appsettings file.
        /// </summary>
        private sealed class LLamaSharpConfigExt : LLamaSharpConfig
        {
            public LLamaSharpConfigExt()
                : base(string.Empty)
            {
            }
        }

... and use this LLamaSharpConfigExt instead to read from the configuration settings.

That's alright with me, but perhaps we could:

  • either consider avoiding adding constructors with parameters on types that can be read from configuration settings
  • or consider defining separate configuration setting types
  • or just do nothing at all since it's up to the library users to decide what is to be put in the configuration.

vvdb-architecture avatar Feb 07 '24 12:02 vvdb-architecture

The problem here is that ModelPath is a non nullable string, that means that after constructing the LlamaSharpConfig object ModelPath must not be null.

That's going to need a constructor of some kind. Or marking the property as required, but I think that would have the same issue with deserialisation?

martindevans avatar Feb 07 '24 13:02 martindevans

I understand. Personally I use IValidateOptions<TOptions> or IValidatableObject in those cases, to separate the definition from its validation.

vvdb-architecture avatar Feb 07 '24 17:02 vvdb-architecture