Update clip.cpp

[Tejaakshaykumar]

Added a comprehensive validation check to ensure that critical hyperparameters (hidden_size, n_head, n_layer, image_size, patch_size, projection_dim, eps) are not set to invalid or zero values.

a throw statement to handle invalid hyperparameter values by raising an invalid_argument exception.

Enhanced the error message within the catch block to log the specific exception message, aiding in debugging.

This is my first contribution in GitHub, and I'm excited to be part of the community!

• I have read the contributing guidelines
• Self-reported review complexity:
  • Low
  • Medium
  • High

#7073

[Galunid]

Positive hparams.patch_size will cause the program to fail, same for other fields without == 0.

[HanClinto]

Welcome to the project -- we're happy to have you here!

For consistency with the rest of the codebase, please use fprintf instead of output streams.

Suggested change

	std::cerr << "Error while loading hyperparameters: " << e.what() << std::endl;
	fprintf(stderr, "Error while loading hyperparameters: %s\n", e.what());

[Tejaakshaykumar]

Positive hparams.patch_size will cause the program to fail, same for other fields without == 0.

I wanted to clarify your comment about hparams.patch_size and the other fields. Should these fields always be non-zero, or are there cases where a positive value might cause issues?

I've added the == 0 checks but want to ensure I'm handling them correctly. Any further insight would be appreciated!

[HanClinto]

So generally, one thing that strikes me about this PR is that the new failure mode doesn't add a ton of information. Whether the program fails early, or whether it fails late, it still is a pretty generic fail message, and while it does give you a bit of new information (the user has an invalid hyperparameter), it doesn't tell you which parameter is out of bounds, nor what the valid range is.

A robust solution would perhaps add bounds-checks to individual parameters (to add min and/or max values), and check each of them (perhaps with a #define macro to save time), but as it is, I wonder if there's an elegant way to inform the user which parameter is causing the problem and assist them more in fixing it.

I'm not interested in adding a ton of code, and I'm willing to admit that my dream of an elegant and generic one-size-fits-all-cases solution to this problem may not exist.

As someone who isn't terribly familiar with clip models, would you be willing to share some of the motivation behind this change? I.E., what situation did you run into that prompted this change?

Thank you!

[Galunid]

I wanted to clarify your comment about hparams.patch_size and the other fields. Should these fields always be non-zero, or are there cases where a positive value might cause issues?

The issue was that in most languages positive values evaluate to true in if statements. In your previous code

if (hparams.hidden_size == 0 || hparams.n_head == 0 || hparams.n_layer == 0 || hparams.n_intermediate || hparams.image_size == 0 || hparams.patch_size || hparams.projection_dim || hparams.eps)

hparams.n_intermediate for example was lacking any sort of comparison, which means it was converted to boolean value. That means if it was bigger than 0 it would become true, and if it was lower or equal to 0 it would become false. Obviously it is incorrect. I think what you want to check is whether all of those values are <= 0.

n_head can be a zero for non-transformer models, but clip doesn't support those so it's fine.

You also have a leftover bracket from try/catch block you removed, and the indentation is incorrect. Please note that in this case try/catch is necessary since models can miss those values, in which case get_u32 will throw an exception.

Another thing is replacing fprintf with LOG_TEE (that's what's used in clip.cpp, llama.cpp uses fprintf) ;)

Just in case, the "request changes" is there to let other maintainers know not to merge this yet and is not meant as a critique of you or the code.

TL;DR
Use <= and bring back try/catch but without throws, replace fprintf with LOG_TEE

[HanClinto]

Another thing is replacing fprintf with LOG_TEE (that's what's used in clip.cpp, llama.cpp uses fprintf) ;)

Aaah, I was wondering when one should be used vs. the other. Thank you, @Galunid ! :) TIL

[Tejaakshaykumar]

Thank you both for your feedback!

I appreciate @HanClinto your comments on the clarity of error messages.The motivation for this change was to handle cases where invalid hyperparameters were leading to ambiguous failures, and I’ll focus on making the error reporting more informative to aid in debugging.

Thank you @Galunid for clarifying the issue with the condition checks. I’ll correct the checks to use <= 0 as needed and address the indentation and leftover bracket issues. I’ll also ensure that fprintf is replaced with LOG_TEE to maintain consistency with the rest of the codebase.

I’ll make these updates and resubmit the code. Thanks again for your help :)

[Tejaakshaykumar]

Hi @HanClinto, @Galunid,

I wanted to follow up on the commit I submitted a few days ago with the changes we discussed. Could you please review it when you have time and let me know if any further updates are needed?

[Tejaakshaykumar]

Deleted optional logs