Skip to content
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

cannot pass string values to the report_to variable after init constructor #34445

Open
3 of 5 tasks
alpertunga-bile opened this issue Oct 27, 2024 · 6 comments · May be fixed by #35403
Open
3 of 5 tasks

cannot pass string values to the report_to variable after init constructor #34445

alpertunga-bile opened this issue Oct 27, 2024 · 6 comments · May be fixed by #35403

Comments

@alpertunga-bile
Copy link
Contributor

alpertunga-bile commented Oct 27, 2024

System Info

  • transformers version: 4.47.0.dev0
  • Platform: Windows-11-10.0.26100-SP0
  • Python version: 3.12.5
  • Huggingface_hub version: 0.26.1
  • Safetensors version: 0.4.5
  • Accelerate version: 1.0.1
  • Accelerate config: not found
  • PyTorch version (GPU?): 2.5.0+cu124 (True)
  • Tensorflow version (GPU?): not installed (NA)
  • Flax version (CPU?/GPU?/TPU?): not installed (NA)
  • Jax version: not installed
  • JaxLib version: not installed
  • Using distributed or parallel set-up in script?:
  • Using GPU in script?:
  • GPU type: NVIDIA GeForce GTX 1660 Ti

Who can help?

@muellerzr @SunMarc

Information

  • The official example scripts
  • My own modified scripts

Tasks

  • An officially supported task in the examples folder (such as GLUE/SQuAD, ...)
  • My own task or dataset (give details below)

Reproduction

  1. Install wandb package
  2. Run the below script with any model (model initialization is not added here for brevity)
default_train_args = TrainingArguments(output_dir="tmp")

default_train_args.report_to = "none"

trainer = Trainer(model=model, args=default_train_args)

Expected behavior

The script will run without any integration when report_to value is assigned with none value.

Problem

The check for the report_to variable is done in the post_init function of the TrainingArguments. If you don't specify it in the constructor then the code set to all and fetch all the integrations. If you assing a value to report_to variable after initialization constructor, the below issue is happening.

In the Trainer constructor, with get_reporting_integration_callbacks function the class is fetching the integration callbacks but the function is not checking if string or array is the type of the report_to value. Because of that the functions raises ValueError if you assign string to the value because it is trying to check with char rather than string. This behaviour is same for the none and all string values.

  • The responsible code parts for the issue can be found below:
    • [code] TrainingArguments
    • [code] Trainer
    • [code] Integration Utils

Solution

Adding below code to the beginning of the get_reporting_integration_callbacks function will fix the issue:

if report_to is None:
     return []

if isinstance(report_to, str):
    if "none" == report_to:
        return []
    elif "all" == report_to:
        report_to = get_available_reporting_integrations()
    else:
        report_to = [report_to]
  • If the issue is approved, I will open PR.
@alpertunga-bile alpertunga-bile changed the title cannot pass string values to the report_to variable for TrainingArguments cannot pass string values to the report_to variable after init constructor Oct 27, 2024
@LysandreJik
Copy link
Member

cc @muellerzr WDYT?

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@SunMarc
Copy link
Member

SunMarc commented Nov 27, 2024

Hey @alpertunga-bile, is there any reason why you want to set report_to after initializing default_train_args ?

@alpertunga-bile
Copy link
Contributor Author

alpertunga-bile commented Nov 27, 2024

Hello @SunMarc, I am training 5 models with a single script in Google Colab, so I need to change the values ​​according to the selected model. Since there are multiple condition checks and the checks affect multiple values; I am assigning them after the init ctor.

Before the updates I had no problem but later it tries to use the wandb package since it is pre-installed in Google Colab. I followed the documentation and since I am following this assignment pattern I tried assigning the value none to the return_to variable after the init ctor then this issue occurred.

I think the solution will provide flexibility and protection from the side effect of the constructor to the user who follows the documentation and uses this assignment pattern. However it seems that this is a rare issue and assignment pattern so the solution will not have any impact on the user experience.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@SunMarc
Copy link
Member

SunMarc commented Dec 23, 2024

I'm okay with adding this to get_reporting_integration_callbacks @alpertunga-bile . Feel free to open a PR !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants