Convert _VALID_DICT_FIELDS to class attribute for shared dict parsing in subclasses

[Tavish9]

What does this PR do?

This PR refactors the _VALID_DICT_FIELDS attribute into a ​class member variable within the base class, enabling subclasses of the training argument class to inherit and reuse a unified dictionary parsing logic in their __post_init__ methods.

Subclasses no longer need to reimplement repetitive __post_init__ logic for dictionary fields.

class SubClassTrainingArguments(TrainingArguments):
    _VALID_DICT_FIELDS = TrainingArguments._VALID_DICT_FIELDS + ["sub_class_dict"]

Fixes huggingface/trl#3082

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.

@muellerzr and @SunMarc

[github-actions]

Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. When it is ready for review, please click the Ready for review button (at the bottom of the PR page).

[Tavish9]

Hi @SunMarc,

any suggestions for this?

[Rocketknight1]

Since this is Trainer/TrainingArguments, leaving it to @SunMarc and @muellerzr!

[muellerzr]

Thanks, if it alleviates a painpoint for TRL/other downstream libs, I don't see an issue here!

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[Tavish9]

@SunMarc, kindly request for review🤗

[SunMarc]

SGTM !

[Tavish9]

Hi all, kindly request for merging

[Rocketknight1]

Hi @Tavish9, we're rebasing/merging your branch onto main to get tests to pass, because of CI issues on the previous commit that your branch was based off. As soon as we can get the CI to pass, we'll merge your branch into main!

[Rocketknight1]

Hi @Tavish9 after rebasing, I think some of the failing tests are actually caused by this PR! Can you check the tests in tests_hub / tests_non_model and see if you can fix them?

[Tavish9]

ok, i'm glad to help the test

[Tavish9]

Hi, @Rocketknight1, I found that the original logic in tests_hub is wrong. I will fix them all.🤗

[Tavish9]

Small testcase:

import unittest
from dataclasses import dataclass, field
from typing import Optional, Union, get_origin

from transformers import HfArgumentParser


@dataclass
class Test:
    accelerator_config: Optional[Union[dict, str]] = field(
        default=None,
        metadata={
            "help": (
                "Config to be used with the internal Accelerator object initialization. The value is either a "
                "accelerator json config file (e.g., `accelerator_config.json`) or an already loaded json file as `dict`."
            )
        },
    )


class HfArgumentParserTest(unittest.TestCase):
    def test_a(self):
        # First find any annotations that contain `dict`
        fields = Test.__dataclass_fields__

        print(fields["accelerator_config"].type)

        parser = HfArgumentParser(Test)

    def test_b(self):
        # First find any annotations that contain `dict`
        fields = Test.__dataclass_fields__

        print(fields["accelerator_config"].type)
        print(get_origin(fields["accelerator_config"].type))

typing.Union[dict, str, NoneType]
.<class 'str'>
None

After HfArgumentParser, the results are not what we want.

We either, move test_integration_training_args to the last, or add additional type checking.

[Rocketknight1]

cc @SunMarc @muellerzr can you take another look now that the tests have been updated as well? You can merge if you're happy

[SunMarc]

Thanks !

[Tavish9]

please check the tests carefully, as we are not aligned with the testing pipeline.

merge if you think there is no problem:)

[Tavish9]

Any updates? @SunMarc, @Rocketknight1

[ydshieh]

Good for me regarding the test. For src/transformers/training_args.py, I believe the other 2 reviewers' approval.

A tiny question, would the change will break current usecase? I see you mentioned

Subclasses no longer need to reimplement repetitive post_init logic for dictionary fields.

Would those codes have to access the module level's _VALID_DICT_FIELDS (that is removed in this PR)?

[SunMarc]

Would those codes have to access the module level's _VALID_DICT_FIELDS (that is removed in this PR)?

Potentially but this is a private attribute so it shouldn't be too big of an issue

[Tavish9]

Would those codes have to access the module level's _VALID_DICT_FIELDS (that is removed in this PR)?

Good question! However, from a naming perspective(semi-private), it doesn't make sense for a subclass to access the parent class's _VALID_DICT_FIELDS. Although, the current logic can be implemented with the following code:

from transformers.training_args import _VALID_DICT_FIELDS
_VALID_DICT_FIELDS.append("my_dict_field")

It works, but I think no one does that.

you can also refer to this discussion for reference.