Don't default to other weights file when use_safetensors=True

[amyeroberts]

What does this PR do?

Fixes #31649

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.

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

[troy-baker-aumni]

Looks good! Thank you so much for your help! @amyeroberts
It is so, so appreciated.

[ydshieh]

Maybe use if from_tf and ... here, to correspond to (several lines above)

            if is_local:
                if from_tf and os.path.isfile(
                    os.path.join(pretrained_model_name_or_path, subfolder, TF_WEIGHTS_NAME + ".index")
                ):

[ydshieh]

same

[ydshieh]

@amyeroberts Thank you. Although I think the changes make some sense, I am not 100% sure if the current design is explicitly aiming to fail only when it tries to load from a remote repository. And when it is local, it is designed to not fail (i.e. always trust a local file).

Therefore I would prefer @LysandreJik to confirm it is a design or it is just a miss.

[amyeroberts]

@ydshieh Thanks for the review!

And when it is local, it is designed to not fail (i.e. always trust a local file).

That's a good point. If that's the case, then I think this is a bit more involved, as I think we'd need to update documentation to make it clearer and possibly revise some of the flags in from_pretrained as this means trust_remote_code, local_files_only and use_safetensors have overlap in control. I think the behaviour is clearer if use_safetensors just controls whether .safetensors files are used.

[ydshieh]

Yes, I agree.

[LysandreJik]

Makes sense! Thanks for your PR @amyeroberts !

[LysandreJik]

And regarding the two messages above, I don't think we want to

And when it is local, it is designed to not fail (i.e. always trust a local file).

We want local execution to behave exactly the same as it would with a remote repository. Otherwise, it can get very messy IMO.

[ydshieh]

I think it would be good (to all of us) that we write a short comment about why we add a test.

As I am sure we already have some tests that are about saving/loading. And when we add these 2 new tests, the main test case is about failing the loading if a format is specified (as we don't switch to other format).

Reading the test names here doesn't tell me what is the main objective, and I will be confused why we still add new saving/loading tests.

[ydshieh]

I only have 2 minor comments otherwise it's good!

(Comments for the tests' goal would be very nice.)