unitaryai / detoxify

Trained models & code to predict toxic comments on all 3 Jigsaw Toxic Comment Challenges. Built using ⚡ Pytorch Lightning and 🤗 Transformers. For access to our API, please email us at contact@unitary.ai.
https://www.unitary.ai/
Apache License 2.0
935 stars 114 forks source link

Bump transformers from 4.30.0 to 4.33.2 #93

Closed dosatos closed 8 months ago

dosatos commented 1 year ago

Bumps transformers from 4.30.0 to 4.33.2.

dosatos commented 11 months ago

can anyone see the request? @laurahanu ?

kraktus commented 11 months ago

I mean understanding why the tests fail and fix them if not a fluke would be necessary before merging anyway.

jamt9000 commented 11 months ago

For some reason the string "None" is being used to load the model

E           requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: 
https://huggingface.co/None/resolve/main/config.json
E           OSError: None is not a local folder and is not a valid model identifier listed on 'https://huggingface.co/models'
E           If this is a private repository, make sure to pass a token having permission to this repo either by logging in with `huggingface-cli login` or by passing `token=<your_token>`

Sounds like it could be a regression of https://github.com/huggingface/transformers/issues/16672

laurahanu commented 11 months ago

Solution inspired by this related issue. Apparently loading the config with AutoConfig works, however, the equivalent with the model class doesn't and results in the same error:

# works
config = AutoConfig.from_pretrained(model_type, num_labels=num_classes)
model = model_class.from_pretrained(
      pretrained_model_name_or_path=None,
      config=huggingface_config_path or config,
      state_dict=state_dict,
      local_files_only=huggingface_config_path is not None,
  )
# doesn't work
config = model_class.from_pretrained(model_type, num_labels=num_classes)
model = model_class.from_pretrained(
      pretrained_model_name_or_path=None,
      config=huggingface_config_path or config,
      state_dict=state_dict,
      local_files_only=huggingface_config_path is not None,
  )
ERROR tests/test_models.py - OSError: None is not a local folder and is not a valid model identifier listed on 'https://huggingface.co/models'
Vela-zz commented 10 months ago

Solution inspired by this related issue. Apparently loading the config with AutoConfig works, however, the equivalent with the model class doesn't and results in the same error:

# works
config = AutoConfig.from_pretrained(model_type, num_labels=num_classes)
model = model_class.from_pretrained(
      pretrained_model_name_or_path=None,
      config=huggingface_config_path or config,
      state_dict=state_dict,
      local_files_only=huggingface_config_path is not None,
  )
# doesn't work
config = model_class.from_pretrained(model_type, num_labels=num_classes)
model = model_class.from_pretrained(
      pretrained_model_name_or_path=None,
      config=huggingface_config_path or config,
      state_dict=state_dict,
      local_files_only=huggingface_config_path is not None,
  )
ERROR tests/test_models.py - OSError: None is not a local folder and is not a valid model identifier listed on 'https://huggingface.co/models'

I think these two code section is not equal because in latest transformers, if config is not a instance of pretrainedConifg, transformers would try to download/find cached file first, at that situation, pretrained_model_name_or_path can not be None. https://github.com/huggingface/transformers/blob/514de24abfd4416aeba6a6455ad5920f57f3567d/src/transformers/modeling_utils.py#L2598-L2616

change

model_class.from_pretrained(model_type, num_labels=num_classes)
->
model_class.config_class.from_pretrained(model_type, num_labels=num_classes)

will make these two equal, same as transformers done https://github.com/huggingface/transformers/blob/514de24abfd4416aeba6a6455ad5920f57f3567d/src/transformers/modeling_utils.py#L2758C9-L2774C14

voarsh2 commented 8 months ago

So.... this isn't being merged?

laurahanu commented 8 months ago

Thanks for looking into this @Vela-zz, just merged your PR, this should be fixed now!