Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add deep copy for consumers #86

Merged
merged 7 commits into from
Jan 17, 2024
Merged

Add deep copy for consumers #86

merged 7 commits into from
Jan 17, 2024

Conversation

sonalmahajan15
Copy link
Contributor

This PR modifies the consumers to add support for deep copy when the consumers are being copied in backpropagation.

[depends on #80 ]

@sonalmahajan15 sonalmahajan15 mentioned this pull request Oct 19, 2023
Merged
@sonalmahajan15 sonalmahajan15 force-pushed the sonalmahajan15/revamp-producer-consumer branch from 9b0b5d3 to daae825 Compare October 20, 2023 17:14
yuxincs
yuxincs reviewed Oct 23, 2023
View reviewed changes
Copy link
Contributor

@yuxincs yuxincs left a comment

Choose a reason for hiding this comment

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

Overall this LGTM modulo two places where the original object is modified in Copy methods. Also, I think it would be good if we have some unit tests for better coverage and guard against the the typos.

annotation/consume_trigger.go Outdated
Comment on lines 101 to 102
copyConsumer := *t
t.Ann = t.Ann.copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
copyConsumer := *t
t.Ann = t.Ann.copy()
copyConsumer := *t
copyConsumer.Ann = t.Ann.copy()

Right? it's a bit strange for a Copy method to return the original Ann and then modify the receiver's Ann.

Can we also add some unit tests (with the help of reflection) to guard against such cases?

The idea I think would be to use cmp.Diff to check the copied objects are deeply equal, and then use reflections to check (1) the pointer fields in the old and new objects are different, and (2) the old objects are not modified (perhaps by comparing them to another copy made by reflect.Copy).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Although not affecting the functionality, this was definitely a type that deserved fixing. You are right, we should unit tests to catch such inadvertent mistakes. Added unit tests for the same under copy_test.

annotation/consume_trigger.go Outdated
Comment on lines 145 to 146
copyConsumer := *t
t.Ann = t.Ann.copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the above here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(same as above. Fixed and added unit test to safeguard :) )

@sonalmahajan15 sonalmahajan15 force-pushed the sonalmahajan15/revamp-producer-consumer branch from 337055f to 019ced5 Compare November 6, 2023 03:54
@codecov Codecov
Copy link

codecov bot commented Nov 6, 2023 •
edited

Codecov Report

Attention: 93 lines in your changes are missing coverage. Please review.

Comparison is base (6814686) 89.29% compared to head (302cf5e) 89.18%.

Files Patch % Lines
annotation/consume_trigger.go 83.57% 69 Missing ⚠️
assertion/function/assertiontree/backprop.go 70.00% 8 Missing and 4 partials ⚠️
util/asthelper/asthelper.go 80.32% 7 Missing and 5 partials ⚠️
Additional details and impacted files 
@@                             Coverage Diff                             @@
##           sonalmahajan15/revamp-producer-consumer      #86      +/-   ##
===========================================================================
- Coverage                                    89.29%   89.18%   -0.11%     
===========================================================================
  Files                                           54       55       +1     
  Lines                                         8688     9156     +468     
===========================================================================
+ Hits                                          7758     8166     +408     
- Misses                                         774      825      +51     
- Partials                                       156      165       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yuxincs
yuxincs approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@yuxincs yuxincs left a comment

Choose a reason for hiding this comment

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

LGTM apart from one comment to separate the test utility stuff to a separate package for easier maintenance.

I think the test utilities here are general and complex enough to warrant being put in a separate test utility package (which is also what stdlib does), but that can definitely be in a separate follow-up PR.

annotation/copy_test.go

type CopyTestSuite struct {
suite.Suite
initStructs []any
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably make this a generic struct with a type constraint on having Copy method to remove some boilerplate code below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue #172 to perform this refactoring in a follow-up PR.

annotation/copy_test.go
// limitations under the License.

package annotation

Copy link
Contributor

Choose a reason for hiding this comment

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

A continuation of the comment made to previous PR: after seeing this, I think we might be more incentivized to move the test utility stuff to a separate package (say, internal/nilawaytest) for easier maintenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue #172 to perform this refactoring in a follow-up PR.

@sonalmahajan15 sonalmahajan15 force-pushed the sonalmahajan15/revamp-producer-consumer branch from 019ced5 to 06ebb58 Compare November 27, 2023 20:29
@sonalmahajan15 sonalmahajan15 force-pushed the sonalmahajan15/revamp-producer-consumer branch from 06ebb58 to 9534584 Compare November 29, 2023 19:22
@sonalmahajan15 sonalmahajan15 force-pushed the sonalmahajan15/revamp-producer-consumer branch from 9534584 to c7b4d03 Compare November 30, 2023 20:02
@sonalmahajan15 sonalmahajan15 force-pushed the sonalmahajan15/revamp-producer-consumer branch from c7b4d03 to 35156bd Compare December 12, 2023 00:33
@sonalmahajan15 sonalmahajan15 force-pushed the sonalmahajan15/revamp-producer-consumer branch from 35156bd to 6814686 Compare January 17, 2024 21:14
@sonalmahajan15 sonalmahajan15 mentioned this pull request Jan 17, 2024
Open
@sonalmahajan15
Refine backprop assignments in error message (#87)
302cf5e 
This PR refines the error message by incorporating information about the
assignments. Following is an example of an informative error message:

Code snippet:
```
func test(x *int) {
	x = nil
	y := x
	z := y
	print(*z)
}
```

Error message:
```
Potential nil panic detected. Observed nil flow from source to dereference point: 
   -> errormessage/errormessage.go:32:9: literal `nil` dereferenced via the assignment(s):
        -> `nil` to `x` at errormessage/errormessage.go:29:2,
        -> `x` to `y` at errormessage/errormessage.go:30:2,
        -> `y` to `z` at errormessage/errormessage.go:31:2
```

[closes #83 ]
[depends on #86 ]
@sonalmahajan15 sonalmahajan15 merged commit b2995c6 into sonalmahajan15/revamp-producer-consumer Jan 17, 2024
5 checks passed
@sonalmahajan15 sonalmahajan15 deleted the sonalmahajan15/add-deep-copy branch January 17, 2024 21:37
@sonalmahajan15 sonalmahajan15 mentioned this pull request Jan 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuxincs yuxincs yuxincs approved these changes

@lazaroclapp lazaroclapp Awaiting requested review from lazaroclapp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sonalmahajan15 @yuxincs