-
Notifications
You must be signed in to change notification settings - Fork 479
Cleaned up non-important warnings from the error list #1111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Martin Evans <[email protected]>
|
Thanks for doing this. We've been needing cleanup here for a long time! |
| _embed_inps = state.EmbedInps!.ToList(); | ||
| _is_prompt_run = state.IsPromptRun; | ||
| _consumedTokensCount = state.ConsumedTokensCount; | ||
| _embeds = state.Embeds.ToList(); | ||
| _last_n_tokens = new FixedSizeQueue<LLamaToken>(state.LastTokensCapacity, state.LastTokens); | ||
| _inp_pfx = state.InputPrefixTokens; | ||
| _inp_sfx = state.InputSuffixTokens; | ||
| _embeds = state.Embeds!.ToList(); | ||
| _last_n_tokens = new FixedSizeQueue<LLamaToken>(state.LastTokensCapacity, state.LastTokens!); | ||
| _inp_pfx = state.InputPrefixTokens!; | ||
| _inp_sfx = state.InputSuffixTokens!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do have to say that you just made a lot of suppressing just to ignore the warnings, which might introduce some non indicative NullReferenceExceptions. I believe the warnings should rather remind to us provide a different behavior when its actually null, or at least provide some information to the user (or logger).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have misunderstood the whole “nullables” concept, but from what I understand, they exist so developers can double check they indeed handle a possible null value properly before implementing it on a new system, and mark with “!” if they indeed did.
In reality, those values will NOT be null here based on all current implementations that the engine supports, and it doesn’t support alternative state loading ways in which these can be null.
Let me know if you have a different view on the nullables, or if you have an alternative way to propose that handles this — or if you wanna try your hand at it I’d be happy to learn how you’ll handle this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m thinking more long-term. If the implementation ever changes, or someone modifies the state-loading logic in the future, we might suddenly start seeing NullReferenceExceptions in unexpected places.
As for this specific comment context, state.Embeds is of a non nullable type LLamaToken[] so I don't even think a suppression was necessary. As of state.InputPrefixTokens, if it can't be null in the current project implementation - why not just change its type to not nullable?
In general I agree with the inhouse double check approach you mentioned, I just hate to assume something will never happen so I made this comment because that was (almost) the only way you handled this warning across the PR, which made me wonder if a "double check" was made. Thank you for your contribution.
LLamaSharp had a lot of warnings that appear on each PR, and bloat the error list.
Some of them are valid, but most of them are just nullables stuff, or missing documentations.
This PR takes care of the non-important warnings, bumping the visibility of current and future important ones.
For reference, here is how it looks on current master:

And here are the warnings that remain after this PR -- only obsolete warnings and warnings about await:
