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

adding trust_remote_code argument for loading dataset in controlnet traininig example #10727

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

Conversation

YanivDorGalron
Copy link
Contributor

changes mode to both the readme and the train_controlnet.py example

@asomoza
Copy link
Member

asomoza commented Feb 10, 2025

Hi, thanks for your contribution, can you help me understand why is this needed? do you have a use case for enabling remote code inside a dataset?

Also it should be False by default not True, so only people that needs it and know what they're doing can use it and enable it.

@YanivDorGalron
Copy link
Contributor Author

Hi,

Thank you for reviewing my pull request.

I followed the instructions in the README for training ControlNet with the fill50k circles dataset on my multi-GPU setup. During this process, I encountered a prompt asking if I trust_remote_code (actually multiple identical prompt for each gpu). Due to this multiprocessing, I couldn't respond 'yes' to the prompt which was the result of a remote code on the dataset side. Considering this example is specific to the fill50k dataset, I assumed setting trust_remote_code=True by default was appropriate.

@asomoza
Copy link
Member

asomoza commented Feb 10, 2025

oh I see, I still haven't had the opportunity to train a ControlNet, so I didn't know that the demo and that dataset need the remote code, its weird that people didn't open this issue before, ccing @sayakpaul since I don't have enough experience with this yet.

@sayakpaul
Copy link
Member

@YanivDorGalron thanks for your PR. Could you show us for which ControlNet checkpoint this is useful?

@YanivDorGalron
Copy link
Contributor Author

Hi @sayakpaul , I run accelerate config default and right after that this command:

export MODEL_DIR="stable-diffusion-v1-5/stable-diffusion-v1-5"
export OUTPUT_DIR="."

accelerate launch train_controller.py \
    --pretrained_model_name_or_path=$MODEL_DIR \
    --output_dir=$OUTPUT_DIR \
    --dataset_name=fusing/fill56k \
    --resolution=512 \
    --learning_rate=1e-5 \
    --validation_image "../conditioning_image_1.png" "./conditioning_image_2.png" \
    --validation_prompt "red circle with blue background" "cyan circle with brown floral background" \
    --train_batch_size=4

@sayakpaul
Copy link
Member

That checkpoint should be well supported. I don't understand why do we need trust_remote_code.

@YanivDorGalron
Copy link
Contributor Author

The trusted_remote_code is needed for the fill50k dataset. The one that the example uses

)
else:
if args.train_data_dir is not None:
dataset = load_dataset(
args.train_data_dir,
cache_dir=args.cache_dir,
trust_remote_code=args.trust_remote_code
Copy link
Member

Choose a reason for hiding this comment

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

I guess we could condition this by checking if we're using the fill-50k dataset otherwise it looks like a bit security-harming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could set the default to be false but write the correct command in the readme. (which is speciific for fill50k). does that sound appropriate?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds perfect to me! Thanks so much!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the suggested modification.
now at every command where fill50k is used a new flag argument was added:

 --dataset_name=fusing/fill50k \ 
 --trust_remote_code \

in addition i made the needed modification for the relevant py files to include the argument
its a store_true argument therefore by default is False.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@sayakpaul
Copy link
Member

@YanivDorGalron can you run make style && make quality?

@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.

@YanivDorGalron
Copy link
Contributor Author

@YanivDorGalron can you run make style && make quality?

These commands modify files unrelated to this PR. Should I proceed with pushing those changes as well?

        modified:   examples/controlnet/train_controlnet_flux.py
        modified:   examples/research_projects/geodiff/geodiff_molecule_conformation.ipynb
        modified:   examples/research_projects/gligen/demo.ipynb

@sayakpaul
Copy link
Member

You can only add git add examples/controlnet/train_controlnet_flux.py and commit the results

@YanivDorGalron
Copy link
Contributor Author

done!

@YanivDorGalron
Copy link
Contributor Author

@sayakpaul I noticed that the PR failed some tests. I'm not sure if the failures are related to my changes— is there anything I can do on my end?

Copy link
Contributor

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.

@github-actions github-actions bot added the stale Issues that haven't received updates label Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues that haven't received updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants