Save PyTorch frontend state in object

[t-vi]

While the functional approach is pretty neat, we ended up having global state (default frontend, dtype) and it'll be more soon (caching of inferred types, see #6900). To not have to pass around the state, this moves the op conversion into a class with instances having the state.

[t-vi]

I've tried to keep this "somewhat minimal", so I didn't move around all the helper functions.
Personally, I think it'd be neat to make the dispatching of the ops into getattr and having functions for them all, but I realize that this is 1) a style thing and 2) somewhat orthogonal to this change, so I'm leaving it out.
And I'm happy about shaving off a few lines, not all of them are blank...

@masahi

[masahi]

Thanks for working on this. I have three questions:

1. What is the distinction between op converters method with @staticmethod annotation and the ones without it (the ones which take self as argument)?
2. Can we remove functools.partial stuff? So rather than having def _unary(name, inputs, input_types): etc, can we have something like def _unary(name): return lambda inputs, input_types: ...
3. Do you intend to move functions such as convert_operators, convert_block etc into the class later? These are the functions that currently require passing around "global" variables such as outputs, convert_map, prelude, default_dtype.

cc @siju-samuel We will be doing big refactoring of pytorch frontend.

[masahi]

Right now CI is having an issue, please retrigger after #7025 is merged.

[masahi]

Also, now that we are encapsulating each converter inside a class, I think it is ok to remove underscore _ in each converter method, if you prefer (replace def _ -> def , self._ -> self.). I'm fine either way.

[t-vi]

Thank you for looking at this.

1. I used static methods to not have unused self. I'm not terribly attached to it, if you like the consistency of everything being a regular method.
2. This is again a consistency thing. Right now all methods apply the operation. Moving some back to returning impl_ has us doing two different things (though I'm not opposed). One alternative I'd consider is to actually register a method for all ops we handle (convert_op_aten_mm) and build the conversion dictionary on the fly. For the time being I'll move the things taking extra arguments back to the impl scheme as you suggest.
3. Yes, I thought I'd do the conversion in pieces. If you prefer I can move these now, too. I will tentatively add them to this PR.
4. Good point about _, in particular as leading _ in classes is special in Python...

[masahi]

1. To avoid confusion when we add a new converter, I think we should make everything staticmethod or regular one. Since this class is supposed to be a singleton, staticmethod makes sense to me.
2. Yes, the arguments of each op converter are supposed to be inputs, input_types. If you add another arg like name, inputs, input_types I'd say it is already not consistent anyway. So I prefer lifting name arg to a wrapper function and returning a new impl function. I think being able to remove all the functools.partial(...) is a big plus.
3. Ok we can do that later.

[t-vi]

For 1. I can make all methods nonstatic. I wouldn't know how to reasonably make all methods static.
Part of this is that I don't agree with the singleton.
With convert map moved into the state and late the graph, it's not a singleton anymore.

[masahi]

Maybe I'm missing something, I'm just wondering why, for example, full_like at

tvm/python/tvm/relay/frontend/pytorch.py

Line 610 in 4643967

def full_like(self, inputs, input_types):

is nonstatic while linspace just below is static? By "all" I just meant all op converters (not all methods), I don't see why we cannot make some of op converters static?

[t-vi]

So the object holds some state such as the default_dtype in order to not have to pass it around in the methods. The methods that use this state have to be non-static. The others, which don't use the state, can be static.
But I have made all conversion methods non-static, maybe that helps, too.

[masahi]

Ah I see, thanks.

[t-vi]

So what I didn't do yet is move all utility functions that use the state (default_dtype and prelude) into the class. We could add that or defer.
Also, I wondered if you're attached to excepting the quantized ops in report_missing_conversions and then adding them to the convert_map later in the from_pytorch function. If one moves the addition to convert_map up, one can drop the special casing in the report_missing_conversions. (Also, I'm not sure if one wouldn't add the quantized ops all along, but I did not study the design enough here.)

[masahi]

Also, I wondered if you're attached to excepting the quantized ops in report_missing_conversions and then adding them to the convert_map later in the from_pytorch function. If one moves the addition to convert_map up, one can drop the special casing in the report_missing_conversions.

Yeah we can definitely register quantized ops converter early. I don't think there is a deep reason it is registered late in the current implementation.

[t-vi]

What do you think, add more now or do it later? If the current state is good for you, I'd have a mild tendency to stop here and to a separate PR for further things (including exploiting the new stateful converter for reworking speedup patch I look forward to a lot).

[masahi]

Yes I'll merge this as it is after CI is cleared. We can do other stuff later.

[t-vi]

At last all the CI is happy. Sorry this took so long.

[masahi]

Thanks @t-vi

[t-vi]

Thank you for reviewing & merging, @masahi !