Skip to content

validation check added #1407

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

validation check added #1407

wants to merge 5 commits into from

Conversation

ved1beta
Copy link
Contributor

@ved1beta ved1beta commented May 1, 2025

SUMMARY:
added a validation check in create_modifier
fixes #1226

Copy link

github-actions bot commented May 1, 2025

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed.

Copy link
Collaborator

@kylesayrs kylesayrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ved1beta ! Could you provide more context as to when this code would be useful?

I also see here that valid_fields is undeclared in some cases, you may want to make sure that that variable is declared in all cases, or make explicit the assumption that all modifiers have model fields

@ved1beta
Copy link
Contributor Author

ved1beta commented May 1, 2025

validation check useful when is writing recipes for modifiers. The specific issue being fixed (issue #1226) occurs when users specify invalid or irrelevant arguments in a modifier's configuration. Without this validation, the system silently accepts invalid arguments, which can lead to several issues. Eg
image

@kylesayrs
Copy link
Collaborator

@ved1beta I see! Have you explored Pydantic's extra="forbid" option at all?

class Model(BaseModel):
    x: int

    model_config = ConfigDict(extra="forbid")

@kylesayrs
Copy link
Collaborator

Sorry, I was actually referencing adding the forbid option directly to the Modifier class. This might be simpler than attempting to create a temporary base class as an intermediary

@kylesayrs
Copy link
Collaborator

kylesayrs commented May 13, 2025

I was actually suggesting to add the extra forbid config to the Modifier class, directly. This solution also covers cases where users create modifiers directly, rather than going through the ModifierFactory

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.

Fail when invalid arguments are provided in the recipe for a modifier
2 participants