-
Notifications
You must be signed in to change notification settings - Fork 31.2k
More ReDOS fixes! #36964
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
More ReDOS fixes! #36964
Conversation
|
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. |
ArthurZucker
left a 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.
Happy with config changes!
imports should be tested in terms of speed. Ast is a bit slow as I remember it but for something lile this we can cache / optimize!
cc @LysandreJik
|
|
|
@Rocketknight1 yeah, the |
f52247a to
a6d5eba
Compare
|
One more performance note: Since most Transformers code is contained within classes, we can speed up walking the However, I don't think this is really that important - parsing and walking the tree takes << 1s for me. I'm not even sure it's slower than the regex solution! |
a6d5eba to
0317f64
Compare
ArthurZucker
left a 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.
SGTM then! Parsing a single file is fast!
3378c82 to
ce86970
Compare
ce86970 to
d5534ab
Compare
|
cc @Michellehbn merged! Should be in the next release and then we can close out those issues |
* More ReDOS fixes! * Slight regex cleanup * Cleanup regex replacement * Drop that regex entirely too * The regex didn't match config.json, let's make sure we don't either * Cleanup allowed_value_chars a little * Cleanup the import search * Catch multi-condition blocks too * Trigger tests * Trigger tests
|
Hi Matt, I find the ast-style parsing would introduce an error if a function is import torch.nn as nn
class Model(nn.Module):
def __init__(self):
super().__init__()
self.linear = nn.Linear(10, 10)
def forward(self, x):
if x.size() == 1: # error
raise ValueError("x.size() == 1")
return self.linear(x)When I try to run |
* More ReDOS fixes! * Slight regex cleanup * Cleanup regex replacement * Drop that regex entirely too * The regex didn't match config.json, let's make sure we don't either * Cleanup allowed_value_chars a little * Cleanup the import search * Catch multi-condition blocks too * Trigger tests * Trigger tests

This PR replaces three more problematic regexes. In all cases I eliminate the regex entirely.
When parsing Python files for
importstatements we now rely onast.parse()instead. This is the most complex code but it seems to pass tests.In
chat.pyI think I figured out what the regex was doing and wrote a Python method to duplicate its functionality - cc @gante to confirm I got it right!Finally, the change in
configuration_utils.pywas very straightforward; we could just usestr.startswith()andstr.endswith()instead.