Record function returned types by aktech · Pull Request #78 · data-apis/python-record-api

aktech · 2020-08-24T13:56:28Z

This is an attempt to implement #15 to record function return types.

It delays the call to log_call method and saves the arguments as an attribute (log_call_args) of the Stack class. The Stack now has a previous_stack attribute to check it the previous stack opname was CALL_METHOD, so that delayed call to log_call can be executed with the known returned value.

TODO:

Tracing for all op codes except:
STORE_ATTR: doesn’t returns anything as it sets a value
DELETE_SUBSCR: doesn’t returns anything
STORE_SUBSCR: doesn’t returns anything
BUILD_TUPLE_UNPACK: will return tuple
BUILD_LIST_UNPACK: will always return list
BUILD_SET_UNPACK: will always return set
BUILD_TUPLE_UNPACK_WITH_CALL: will always return tuple
COMPARE_OP: will always return boolean
Do not trace exception return types
Union of return types if a function returns more than one type (for e.g: np.add(1, 1) and np.add(1, 1.2))
Return type hint in the final output

saulshanabrook · 2020-08-24T14:16:06Z

Wow, this is great! I'll take a look.

saulshanabrook

Overall this looks like the right approach! Thank you.

I am curious though how the particular implementation might evolve as we add support for all the other opcodes as well.

I.e. I am thinking instead of storing the whole previous stack we might end up with just storing the opcode and some other structure?

But I think it's hard to know the scope of how general/specific it all should be until we add support for all other opcodes.

How would you feel about continue to work off this same branch on those? I think it's probably easiest to for me to continue reviewing it all as one big PR instead of merging in and then updating it.

saulshanabrook · 2020-08-24T14:19:12Z


 def parse_line(
-    n: int, function: object, params=None, bound_params=None,
+    n: int, function: object, params=None, bound_params=None, return_type=None


This is fine for now. We will have to add something to the signature to represent the return type and then we should display it as a return type annotation, see PEP 484:

def greeting(name: str) -> str: return 'Hello ' + name

It can be anything at the moment, as we don't have a specialised object. Do you mean to say adding Any hint?

Yeah for most opcodes we don't k ow the return type yet, but as you implement them, we will...

The whole "how do we type things and then combine them" is another part of the code, totally separate from the recording.

We can use the ObjectOutput for places we don't know the return type yet. It is the same as typing.Any.

aktech · 2020-08-24T15:24:54Z

I.e. I am thinking instead of storing the whole previous stack we might end up with just storing the opcode and some other structure?

That is totally possible, I started with whole stack in case I need something else, currently I am only using the opname attribute.

How would you feel about continue to work off this same branch on those?

That sounds good to me. 👍

saulshanabrook · 2020-08-24T16:08:02Z

Ah a couple other things to think about (don't need to worry about these now, but wanted to get them down now that I remember them):

We should try this on a function that raises an exception. I think in that case, we should skip tracing that call to the function.
We could/should record from numpy import arange as getattr(numpy, "arange"). And a from numpy import * as a bunch of getattr one for each of __all__ on the module or all values on the modules otherwise? Then, if we also record the return types for each of those as the values, we will be able to understand where people imported things from (Put types in parent modules #6) as well as see where ufuncs were defined (We don't know where ufuncs are from! #70)! The analysis step of this could be done in a follow up PR though.

aktech · 2020-08-28T12:50:17Z

We should try this on a function that raises an exception. I think in that case, we should skip tracing that call to the function.

Wouldn't that halt the execution of the tracing anyways?

saulshanabrook · 2020-08-28T13:50:31Z

Wouldn't that halt the execution of the tracing anyways?

Not necessarily, if it is caught and execution continues.

aktech · 2020-08-28T13:54:33Z

Not necessarily, if it is caught and execution continues.

Oh, I see what you mean. By the way I have added the tracing for normal (non method) function calls too.

saulshanabrook · 2020-09-07T12:53:44Z

+
    # special case subscr b/c we only check first arg, not both
    def op_BINARY_SUBSCR(self):
        self.process((self.TOS1,), op.getitem, (self.TOS1, self.TOS))


Can we record the return type for any of some these special methods as well?

Yes, I think so.

add return type to signature

aktech · 2020-09-15T21:47:40Z

I have included the tracing for all the functions, where it made sense to, here are notes for the ones that didn't make it

STORE_ATTR: doesn’t returns anything as it sets a value
DELETE_SUBSCR: doesn’t returns anything
STORE_SUBSCR: doesn’t returns anything
BUILD_TUPLE_UNPACK: will return tuple
BUILD_LIST_UNPACK: will always return list
BUILD_SET_UNPACK: will always return set
BUILD_TUPLE_UNPACK_WITH_CALL: will always return tuple
COMPARE_OP: will always return boolean

Let me know, if that doesn't makes sense. I have added the signature for the return types as well.

TODO: Merge return types (Union of return types if a function returns more than one type)

aktech · 2020-09-16T07:32:37Z

The test failure is due to this issue: #83

saulshanabrook · 2020-09-16T15:54:51Z

@aktech That list sounds right on, thank you so much for this! I'll work on fixing the CI.

aktech · 2020-09-25T16:35:02Z

@saulshanabrook I have added the return types union as well.

saulshanabrook · 2020-10-05T17:34:12Z

Thank you for working on this! Looks good.

cb9f5ad

aktech requested a review from saulshanabrook August 24, 2020 13:57

saulshanabrook reviewed Aug 24, 2020

View reviewed changes

aktech force-pushed the record-returned-value branch from 297f6c1 to cf8c27c Compare August 28, 2020 12:41

aktech added 4 commits September 7, 2020 12:44

Record function returned types

54dc3e7

Delete previous stack of previous stack

0c8b1a5

Expand record return values for all function calls

4124e66

Skip recording exceptions as return type

1940b76

saulshanabrook reviewed Sep 7, 2020

View reviewed changes

Comment thread record_api/core.py

saulshanabrook reviewed Sep 7, 2020

View reviewed changes

Comment thread record_api/core.py

aktech added 3 commits September 15, 2020 22:17

Add tracing for remaining func calls and

f834575

add return type to signature

Save previous stack only if required

1a9eb3e

change return type pydantic field

0066ca7

aktech force-pushed the record-returned-value branch from c09670e to 0066ca7 Compare September 15, 2020 21:34

Fix type annotations for return type

8e9a08b

saulshanabrook reviewed Sep 16, 2020

View reviewed changes

Comment thread record_api/test.py

aktech added 2 commits September 25, 2020 16:21

Convert return type to OutputType

691565b

Take union of return types for a function

914972c

aktech requested a review from saulshanabrook September 25, 2020 16:34

saulshanabrook merged commit cb9f5ad into master Oct 5, 2020

saulshanabrook deleted the record-returned-value branch October 5, 2020 17:34

saulshanabrook added a commit that referenced this pull request Oct 28, 2020

Revert "Merge pull request #78 from data-apis/record-returned-value"

3a62409

cb9f5ad

saulshanabrook mentioned this pull request Nov 1, 2020

Record return types and determine exposed modules for objects #15

Open

Conversation

aktech commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saulshanabrook commented Aug 24, 2020

Uh oh!

saulshanabrook left a comment

Choose a reason for hiding this comment

Uh oh!

saulshanabrook Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

aktech Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

saulshanabrook Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aktech commented Aug 24, 2020

Uh oh!

saulshanabrook commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aktech commented Aug 28, 2020

Uh oh!

saulshanabrook commented Aug 28, 2020

Uh oh!

aktech commented Aug 28, 2020

Uh oh!

saulshanabrook Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

aktech Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aktech commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aktech commented Sep 16, 2020

Uh oh!

saulshanabrook commented Sep 16, 2020

Uh oh!

Uh oh!

aktech commented Sep 25, 2020

Uh oh!

saulshanabrook commented Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aktech commented Aug 24, 2020 •

edited

Loading

saulshanabrook commented Aug 24, 2020 •

edited

Loading

aktech commented Sep 15, 2020 •

edited

Loading