search

jeongukjae / huggingface-to-tfhub

Converting Huggingface Models to TFHub
5 stars 0 forks source link

Allow bert_pack_inputs to have a configurable seq_length #7

Open rclough opened 2 years ago

rclough commented 2 years ago

Hey there! First off I'd like to thank you on all the work you've done to convert these HF models into TF Hub models, we've found your work really useful within my organization.

We were trying to use the DistilBERT uncased model, and wanted to set a different seq_length for bert_pack_inputs. We noticed on the official tensorflow site, it's conventional to support an optional seq_length for the preprocessor's bert_pack_inputs implementation. Indeed in your underlying implementation, there is the option, but it's not exposed in the call function, which only allows the default length of 128.

For now, we have copied our own version of DistilBertPackInputs, but it would be nice to just use the TF Hub preprocessor as-is.

I was wondering if there was a reason you avoided including it - I'd be happy to open a PR to add it, but I don't have a good means of testing to make sure that the change is backwards compatible with anything else you may be using it for.

rclough commented 2 years ago

Actually looking though your comments in the code it looks like this is theoretically handled in the Save Wrapper, but when using it, I can't seem to use an optional seq_length

rclough commented 2 years ago

If you happen to know how to override the seq_length when loading from TF Hub, that would be really useful info - I can't seem tofigure it out, and using preprocessing.bert_pack_inputs (inputs, seq_length = N) complains that the signature doesnt contain seq_length