Fix 'vocab_size' attribute of LlavaConfig

[ywang96]

What does this PR do?

This PR fixes a bug from #29586 where it made vocab_size a read-only property of LlavaConfig, preventing it from being assigned during the super().__init__(**kwargs). The fix is to pop vocab_size from kwargs before we call super().__init__(**kwargs).

Fixes ##29789

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

[ywang96]

@amyeroberts Could you please take a look since you reviewed the original Llava-1.6 PR?

cc @xwjiang2010

[fxmarty]

related #29389 (comment)

[amyeroberts]

Thanks for fixing this! And apologies for breaking things.

For full backwards compatibility, I think we also need the setter solution from @fxmarty.

This way, vocab_size won't set automatically, but the previous functionality of

config.vocab_size = value

still works

[amyeroberts]

Could you move this to L108? This way we'll more easily remember to remove is when it's fully deprecated

[amyeroberts]

@ywang96 Thanks for opening this PR!

We merged in #29389, which handles the breaking error. Thinking about it more, at the moment, we shouldn't need to pop from the kwargs with the setter solution. Later, once deprecated (i.e. we've had a ~2 months of warnings that it's not used), we'll want to allow users to still pass in this argument, as technically for any config I can do:

config = MyConfigClass(foo=5)

and the param foo will be set, even if it's not a argument of the config. So we really keep vocab_size (even if it is admittedly confusing to have it there).

Closing for now, as the issue is addressed.

[ywang96]

@ywang96 Thanks for opening this PR!

We merged in #29389, which handles the breaking error. Thinking about it more, at the moment, we shouldn't need to pop from the kwargs with the setter solution. Later, once deprecated (i.e. we've had a ~2 months of warnings that it's not used), we'll want to allow users to still pass in this argument, as technically for any config I can do:

config = MyConfigClass(foo=5)

and the param foo will be set, even if it's not a argument of the config. So we really keep vocab_size (even if it is admittedly confusing to have it there).

Closing for now, as the issue is addressed.

Sounds good - I'm okay with either as long as the issue is fixed, and I agree the setter solution is cleaner. Thanks!

[amyeroberts]

@ywang96 A patch v4.39.1 has just been released with the fix