-
Notifications
You must be signed in to change notification settings - Fork 26.6k
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
Avoid import sentencepiece_model_pb2
in utils.__init__.py
#24689
Conversation
@@ -184,9 +184,6 @@ | |||
from . import sentencepiece_model_pb2 | |||
else: | |||
from . import sentencepiece_model_pb2_new as sentencepiece_model_pb2 | |||
else: | |||
# just to get the expected `No module named 'google.protobuf'` error | |||
from . import sentencepiece_model_pb2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this as I would like to keep the following usage
from (transformers).utils import sentencepiece_model_pb2 as model_pb2
gives the same error
No module named 'google.protobuf'
as before 24599
, when protobuf
is not in the env.
It turns out that, if we just ignore this else
block, the above import
statement (when protobuf
is missing) will find the corresponding file, and import it directly, and we wil get the expected error as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, interesting. Just to make sure I've understood, without this else
statement, we still get the correct No module named 'google.protobuf'
error still triggers if protobuf isn't in the env?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The __init__
won't have the name sentencepiece_model_pb2
, but the file utils/sentencepiece_model_pb2.py
exist. So the import will look at that file and try to import.
(I was overthinking, partially I was thinking to have sentencepiece_model_pb2_old.py
and sentencepiece_model_pb2_new.py
(and need to keep the name sentencepiece_model_pb2
available in the __init__
for backward compatibility)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining 🤗
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating!
…gface#24689) fix Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
What does this PR do?
Otherwise, trying to import anything from
utils
will fail if protobuf is not installed.More details in the comment.