-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Fix expecttest accept on Python 3.8 and later #59709
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
Conversation
Fixes #59705. Python 3.8 fixed tracebacks to report the beginning of the line that raised an error, rather than the end. This makes for a simpler implementation (no more string reversing) but need to actually implement. This wasn't caught by tests because we hard coded line numbers to do substitutions, so I also added a little smoketest to detect future changes to traceback line number behavior. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit cb93069 (more details on the Dr. CI page):
2 failures not recognized by patterns:
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Fixes #59705. Python 3.8 fixed tracebacks to report the beginning of the line that raised an error, rather than the end. This makes for a simpler implementation (no more string reversing) but need to actually implement. This wasn't caught by tests because we hard coded line numbers to do substitutions, so I also added a little smoketest to detect future changes to traceback line number behavior. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 52d7025 Pull Request resolved: #59709
|
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Note to reviewers, I don't think either of you are experts in this code, I hope you can just review it on its merits. |
| ]) | ||
|
|
||
| return (src[:i] + RE_EXPECT.sub(replace, src[i:], count=1), delta[0]) | ||
| else: |
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.
more of these two conditions could probably be de-duped? Not a big deal though
| ''') | ||
| """) | ||
|
|
||
| def test_sample_lineno_at_start(self): |
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.
nit:
maybe specifically named previous test in line:49 as
test_sample_lineno_at_end(self)
Fixes #59705. Python 3.8 fixed tracebacks to report the beginning of the line that raised an error, rather than the end. This makes for a simpler implementation (no more string reversing) but need to actually implement. This wasn't caught by tests because we hard coded line numbers to do substitutions, so I also added a little smoketest to detect future changes to traceback line number behavior. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D28994919](https://our.internmc.facebook.com/intern/diff/D28994919) [ghstack-poisoned]
|
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
In #59709 I attempted to fix the expecttest machinery to work in Python 3.8. However, I noticed that it would fail to do substitutions in this case: ``` self.assertExpectedInline( foo(), """bar""" ) ``` This is because the triple quoted string is not on the same line as the backtrace line number (at the very beginning), and for safety reasons the preexisting regex refused to search beyond the first line. This wasn't a big deal prior to Python 3.8 because the flipped version of the regex simply required the triple quoted string to be flush with the end of the statement (which it typically was!) But it is a big deal now that we only have the start of the statement. I couldn't think of a way to fix this in the current model, so I decided to call in the big guns. Instead of trying to do the regex with only the start xor end line number, I now require you provide BOTH line numbers, and we will only regex within this range. The way we compute these line numbers is by parsing the Python test file with ast, and then searching through statements until we find one that is consistent with the line number reported by the backtrace. If we don't find anything, we conservatively assume that the string lies exactly in the backtrace (and you'll probably fail the substitution in that case.) The resulting code is quite a lot simpler (no more reversed regex) and hopefully more robust, although I suppose we are going to have to do some field testing. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
…ood." In #59709 I attempted to fix the expecttest machinery to work in Python 3.8. However, I noticed that it would fail to do substitutions in this case: ``` self.assertExpectedInline( foo(), """bar""" ) ``` This is because the triple quoted string is not on the same line as the backtrace line number (at the very beginning), and for safety reasons the preexisting regex refused to search beyond the first line. This wasn't a big deal prior to Python 3.8 because the flipped version of the regex simply required the triple quoted string to be flush with the end of the statement (which it typically was!) But it is a big deal now that we only have the start of the statement. I couldn't think of a way to fix this in the current model, so I decided to call in the big guns. Instead of trying to do the regex with only the start xor end line number, I now require you provide BOTH line numbers, and we will only regex within this range. The way we compute these line numbers is by parsing the Python test file with ast, and then searching through statements until we find one that is consistent with the line number reported by the backtrace. If we don't find anything, we conservatively assume that the string lies exactly in the backtrace (and you'll probably fail the substitution in that case.) The resulting code is quite a lot simpler (no more reversed regex) and hopefully more robust, although I suppose we are going to have to do some field testing. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D29146943](https://our.internmc.facebook.com/intern/diff/D29146943) [ghstack-poisoned]
…ood." In #59709 I attempted to fix the expecttest machinery to work in Python 3.8. However, I noticed that it would fail to do substitutions in this case: ``` self.assertExpectedInline( foo(), """bar""" ) ``` This is because the triple quoted string is not on the same line as the backtrace line number (at the very beginning), and for safety reasons the preexisting regex refused to search beyond the first line. This wasn't a big deal prior to Python 3.8 because the flipped version of the regex simply required the triple quoted string to be flush with the end of the statement (which it typically was!) But it is a big deal now that we only have the start of the statement. I couldn't think of a way to fix this in the current model, so I decided to call in the big guns. Instead of trying to do the regex with only the start xor end line number, I now require you provide BOTH line numbers, and we will only regex within this range. The way we compute these line numbers is by parsing the Python test file with ast, and then searching through statements until we find one that is consistent with the line number reported by the backtrace. If we don't find anything, we conservatively assume that the string lies exactly in the backtrace (and you'll probably fail the substitution in that case.) The resulting code is quite a lot simpler (no more reversed regex) and hopefully more robust, although I suppose we are going to have to do some field testing. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D29146943](https://our.internmc.facebook.com/intern/diff/D29146943) [ghstack-poisoned]
Summary: Pull Request resolved: #60044 In #59709 I attempted to fix the expecttest machinery to work in Python 3.8. However, I noticed that it would fail to do substitutions in this case: ``` self.assertExpectedInline( foo(), """bar""" ) ``` This is because the triple quoted string is not on the same line as the backtrace line number (at the very beginning), and for safety reasons the preexisting regex refused to search beyond the first line. This wasn't a big deal prior to Python 3.8 because the flipped version of the regex simply required the triple quoted string to be flush with the end of the statement (which it typically was!) But it is a big deal now that we only have the start of the statement. I couldn't think of a way to fix this in the current model, so I decided to call in the big guns. Instead of trying to do the regex with only the start xor end line number, I now require you provide BOTH line numbers, and we will only regex within this range. The way we compute these line numbers is by parsing the Python test file with ast, and then searching through statements until we find one that is consistent with the line number reported by the backtrace. If we don't find anything, we conservatively assume that the string lies exactly in the backtrace (and you'll probably fail the substitution in that case.) The resulting code is quite a lot simpler (no more reversed regex) and hopefully more robust, although I suppose we are going to have to do some field testing. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Test Plan: Imported from OSS Reviewed By: walterddr Differential Revision: D29146943 Pulled By: ezyang fbshipit-source-id: 2c24abc3acd4275c5b3a8f222d2a60cbad5e8c78
Stack from ghstack:
Fixes #59705.
Python 3.8 fixed tracebacks to report the beginning of the line
that raised an error, rather than the end. This makes for a simpler
implementation (no more string reversing) but need to actually
implement. This wasn't caught by tests because we hard coded line
numbers to do substitutions, so I also added a little smoketest to
detect future changes to traceback line number behavior.
Signed-off-by: Edward Z. Yang ezyang@fb.com
Differential Revision: D28994919