Skip to content

Conversation

martindevans
Copy link
Member

@martindevans martindevans commented Sep 6, 2023

I noticed that these properties are unused.

@martindevans martindevans changed the title Removed 3 unused properties of InferenceParams Removed unused properties of InferenceParams Sep 9, 2023
@martindevans martindevans changed the title Removed unused properties of InferenceParams Removed unused properties of InferenceParams & ModelParams Sep 9, 2023
@martindevans
Copy link
Member Author

I'm leaving this open for a while, just in case anyone wants to say that actually we should keep these properties and implement them properly.

@saddam213
Copy link
Collaborator

Wondering if renaming ModelAlias to Name would be beneficial

Not an issuse for LLamaSharp itself, but if you are dealing with multiple models it could be handy to have a identifier/display name other than filepath

This is also something end users can esily add themselfs, so maybe not

@martindevans
Copy link
Member Author

I'm not against adding something like that. I'm only proposing this PR because right now the properties are totally unused, but if you want to open a PR to implement any of these things I'll happily review it.

For the proposed Name property are you thinking it would tag the LLamaWeights object with the name (public readonly property), to make it easier to identify? That's not a bad idea.

@saddam213
Copy link
Collaborator

That what I was thinking, but users could just name thier model files a bit clearer and that would also work

And thinking about it more, Name would really be more of a field requiered if dealing with multiple models, and in that case users would most likely need an Id field too, so they would most likely create thier own classess from the interfaces anyway

Ignore my old man ramblings :p

@martindevans martindevans merged commit b1e9d82 into SciSharp:master Sep 13, 2023
@martindevans martindevans deleted the removed_unused_inference_params branch September 13, 2023 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants