KEMBAR78
Fix expecttest accept on Python 3.8 and later by ezyang · Pull Request #59709 · pytorch/pytorch · GitHub
Skip to content

Conversation

@ezyang
Copy link
Contributor

@ezyang ezyang commented Jun 9, 2021

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

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]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 9, 2021

💊 CI failures summary and remediations

As of commit cb93069 (more details on the Dr. CI page):


  • 4/4 failures possibly* introduced in this PR
    • 2/4 non-scanned failure(s)

2 failures not recognized by patterns:

Job Step Action
CircleCI pytorch_xla_linux_bionic_py3_6_clang9_test Run tests 🔁 rerun
GitHub Actions Lint / mypy Run mypy 🔁 rerun

ci.pytorch.org: 1 failed


This 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.

Click here to manually regenerate this comment.

ezyang added a commit that referenced this pull request Jun 9, 2021
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
Copy link
Contributor Author

ezyang commented Jun 9, 2021

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang ezyang requested review from bdhirsh and walterddr June 9, 2021 15:41
@ezyang
Copy link
Contributor Author

ezyang commented Jun 9, 2021

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:
Copy link
Contributor

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):
Copy link
Contributor

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
Copy link
Contributor Author

ezyang commented Jun 10, 2021

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in 700add0.

ezyang added a commit that referenced this pull request Jun 15, 2021
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]
ezyang added a commit that referenced this pull request Jun 15, 2021
…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]
ezyang added a commit that referenced this pull request Jun 16, 2021
…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]
facebook-github-bot pushed a commit that referenced this pull request Jun 17, 2021
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
@facebook-github-bot facebook-github-bot deleted the gh/ezyang/1036/head branch June 18, 2021 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants