Skip to content

Fix #7751: Use original filename for array exprs - #7752

Merged
sklam merged 1 commit into
numba:masterfrom
gmarkall:issue-7751
Jan 25, 2022
Merged

Fix #7751: Use original filename for array exprs#7752
sklam merged 1 commit into
numba:masterfrom
gmarkall:issue-7751

Conversation

@gmarkall

@gmarkall gmarkall commented Jan 17, 2022

Copy link
Copy Markdown
Member

Using a synthetic filename causes an issue for coverage, which cannot find the synthetic file when generating a report.

This commit restores the use of the original filename for array expressions in order to resolve the issue.

@stuartarchibald I'm not sure about whether this is correct - it fixes coverage, but I couldn't work out from the PR which added the synthetic filename (#7177) why the synthetic filename is used. What are your thoughts here?

Fixes #7751.

@gmarkall
Fix numba#7751: Use original filename for array exprs
e9d0645 
Using a synthetic filename causes an issue for coverage, which cannot
find the synthetic file when generating a report.

This commit restores the use of the original filename for array
expressions in order to resolve the issue.
@gmarkall gmarkall added 2 - In Progress Effort - short Short size effort needed labels Jan 17, 2022
@gmarkall gmarkall added this to the Numba 0.55.1 milestone Jan 17, 2022
@stuartarchibald

stuartarchibald commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

@stuartarchibald I'm not sure about whether this is correct - it fixes coverage, but I couldn't work out from the PR which added the synthetic filename (#7177) why the synthetic filename is used. What are your thoughts here?

I seem to recall changing it to help with debug info. I think setting the filename to expr.loc associates the array expr DI with the file containing the array expr, which can lead to strange behaviours (e.g. nexting in gdb doesn't move the to the next line, step puts to you to line 0 of the expr.loc filename, that sort of thing). To fix this, probably need to find a reproducing problematic case for gdb with this patch as-is and then see what can be done to fix it but keep coverage working.

@gmarkall

gmarkall commented Jan 21, 2022

Copy link
Copy Markdown
Member Author

To fix this, probably need to find a reproducing problematic case for gdb with this patch as-is and then see what can be done to fix it but keep coverage working.

Perhaps an alternative is to suppress emitting debug info for synthetic array expressions, with the aim that the array expression is all seen as part of the source line from which it was generated?

@stuartarchibald

stuartarchibald commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

Perhaps an alternative is to suppress emitting debug info for synthetic array expressions, with the aim that the array expression is all seen as part of the source line from which it was generated?

I think this might be the thing to do. Currently, and invariant of this patch, if you step onto a line with an array expr on it the stepped-to location is the first line of numba.np.npyimpl._broadcast_onto. Then stepping through that and out again leaves you on line 1 of the file containing the array expr. I think for now, changing the filename back to something that coverage is working with is probably ok and also minimally invasive/lowest risk for 0.55.1.

@stuartarchibald

stuartarchibald commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

I've checked locally with coverage that this patch fixes the issue in #7751.

stuartarchibald
stuartarchibald approved these changes Jan 24, 2022
View reviewed changes

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review labels Jan 24, 2022
@stuartarchibald stuartarchibald mentioned this pull request Jan 24, 2022
Open
@sklam
sklam merged commit d96ac34 into numba:master Jan 25, 2022
@stuartarchibald stuartarchibald mentioned this pull request Jan 26, 2022
Merged
stuartarchibald added a commit to stuartarchibald/numba that referenced this pull request Jan 26, 2022
@stuartarchibald
Avoid issue with DI gen for arrayexprs.
ce84d2a 
As title. Alters a test looking for breakpoints on lines that uses
arrayexprs to not use arrayexprs as numba#7752 leads to the arrayexpr
referencing the source line repeatedly.

Fixes numba#7787
@stuartarchibald stuartarchibald mentioned this pull request Jan 26, 2022
Merged
esc pushed a commit to esc/numba that referenced this pull request Jan 26, 2022
@sklam @esc
Merge pull request numba#7752 from gmarkall/issue-7751
f22816d 
Fix numba#7751: Use original filename for array exprs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartarchibald stuartarchibald stuartarchibald approved these changes
@esc esc Awaiting requested review from esc
@sklam sklam Awaiting requested review from sklam

Assignees

No one assigned

Labels

5 - Ready to merge Review and testing done, is ready to merge Effort - short Short size effort needed

Projects

None yet

Milestone

Numba 0.55.1

Development

Successfully merging this pull request may close these issues.

numba 0.55 breaks compatibility with coverage

3 participants

@gmarkall @stuartarchibald @sklam