-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Reset alarm signal when the function is ended #29706
Conversation
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.
Might be hard to reproduce, but yes agree with you we should reset in a finally!
@@ -614,6 +615,10 @@ def resolve_trust_remote_code(trust_remote_code, model_name, has_local_code, has | |||
f"load the model. You can inspect the repository content at https://hf.co/{model_name}.\n" | |||
f"Please pass the argument `trust_remote_code=True` to allow custom code to be run." | |||
) | |||
finally: | |||
if prev_sig_handler is not None: | |||
signal.signal(signal.SIGALRM, prev_sig_handler) |
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.
this will restore to the signal before the try
not to the signal before load_custom_code
. Should we not store signal.getsignal(signal.SIGALRM)
before all that?
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.
Sorry, I didn't find where load_custom_code
is, but I don't think we should reset it here.
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.
Oh no my bad. LGTM
Yeah, it's not easy to reproduce, but I've reproduced it, please see the newest comment in the issue #29690 (comment) |
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 the reproducer and the fix
@@ -614,6 +615,10 @@ def resolve_trust_remote_code(trust_remote_code, model_name, has_local_code, has | |||
f"load the model. You can inspect the repository content at https://hf.co/{model_name}.\n" | |||
f"Please pass the argument `trust_remote_code=True` to allow custom code to be run." | |||
) | |||
finally: | |||
if prev_sig_handler is not None: | |||
signal.signal(signal.SIGALRM, prev_sig_handler) |
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.
Oh no my bad. LGTM
What does this PR do?
Fixes #29690
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?