Add record/replay support for overridden methods, type serializers #241
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
+ Coverage 69.09% 69.66% +0.56%
==========================================
Files 52 52
Lines 3456 3511 +55
==========================================
+ Hits 2388 2446 +58
+ Misses 1068 1065 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
peterallenwebb
changed the title
There was a problem hiding this comment.
This looks great!
I think two things to things about later on:
- write things out in the per thread fashion
- think a bit about memory management(let's not blow up people's existing job because recording we hold in memory takes too much space). We can consider flush things to disk as a node finish.
| def write(self) -> None: | ||
| with open(self.current_recording_path, "w") as file: | ||
| json.dump(self._to_dict(), file) | ||
| self.write_json(file) |
There was a problem hiding this comment.
I am assuming we are going to write to the other folder structure we discussed in a later PR?
| set_invocation_context({}) | ||
| get_invocation_context().recorder = recorder | ||
|
|
||
| recorder.register_serialization_strategy(CustomType, CustomSerializationStrategy()) |
There was a problem hiding this comment.
This is great! I guess we can just maintain a file of type: serialization later on for all of the types we encounter?
| def test_func(self, a: int) -> int: | ||
| return 2 * a | ||
|
|
||
| class RecordableSubclass(Recordable): |
| def test_func(self, a: int) -> int: | ||
| return 2 * a | ||
|
|
||
| class RecordableSubclass(Recordable): |
There was a problem hiding this comment.
I am assuming we have more than 2 layers(baseAdapter, sqlAdapter, SnowflakeAdapter), does it still work?(I think so but a bit harder to reason through, maybe consider expand this test a bit?
There was a problem hiding this comment.
That's a good point, I also need to inject a modification to the subclasses own init_subclass function. I'll add that in the next revision.
Description
In previous versions, the record/replay mechanism did not support function overrides. If a recorded method on class A was overridden by an inheriting class B, calls to the override would not be recorded. This revision fixes that shortcoming.
This revision also allows a mashumaro SerializationStrategy to be registered so that "auto" recorded functions with complex parameters or return values can be handled with a minimum amount of effort.
Checklist
changie newto create a changelog entry