kylebgorman / pynini

Read-only mirror of Pynini
http://pynini.opengrm.org
Apache License 2.0
120 stars 26 forks source link

Python inheritance not reflecting C++'s #20

Closed PierreAndreNoel closed 4 years ago

PierreAndreNoel commented 4 years ago

Some python classes (e.g., Fst) do not reflect the same inheritance as their C++ counterparts, which confuses IDE's and related tooling. See this image for a concrete example (expected behavior: verify is properly recognized as a method of Fst).

unresolved

I'm still new to Cython, but I believe that this specific instance could be fixed by replacing object by _Fst in the definition of Fst. https://github.com/kylebgorman/pynini/blob/76178c3e25d6570b425b2a30f188cf835f7a6f23/src/pywrapfst.pyx#L2844

Other classes may have the same issue.

kylebgorman commented 4 years ago

Maybe that would make your IDE happy, but Fst really doesn't inherit from _Fst.

In fact Fst doesn't meaningfully inherit from anything other than object because it's pure static, as you can see. And, f in your example isn't actually an instance of Fst either, though it is an example of _Fst.

There are very challenging problems making the OpenFst class hierarchy work in a Python space and the use of a dummy class Fst which calls _create_Fst was the solution I came up with. There may be a better alternative but I think it'll require a larger redesign.

On Thu, Jan 9, 2020 at 12:18 PM Pierre-Andre Noel notifications@github.com wrote:

Some python classes (e.g., Fst) do not reflect the same inheritance as their C++ counterparts, which confuses IDE's and related tooling. See this image for a concrete example (expected behavior: verify is properly recognized as a method of Fst).

[image: unresolved] https://user-images.githubusercontent.com/8442124/72088592-81c94f00-32d8-11ea-9a12-4f697a2d4c5c.png

I'm still new to Cython, but I believe that this specific instance could be fixed by replacing object by _Fst in the definition of Fst.

https://github.com/kylebgorman/pynini/blob/76178c3e25d6570b425b2a30f188cf835f7a6f23/src/pywrapfst.pyx#L2844

Other classes may have the same issue.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/20?email_source=notifications&email_token=AABG4OOGAT5PKPDT2O3BD2TQ45L4ZA5CNFSM4KE3YGP2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IFD5QIA, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4ONXWNUJM7BLC6CT3RTQ45L4ZANCNFSM4KE3YGPQ .

PierreAndreNoel commented 4 years ago

There may be a better alternative but I think it'll require a larger redesign.

Do you predict that type hinting and/or stubs could help? There also appears to be some native Cython support toward typing.

I'm still in the process of assessing if pywrapfst and/or pynini are appropriate for my use case but, if things continue the way they have, I may consider contributing some code toward such type hinting (which could be a good opportunity for me to learn Cython). Would you see value in that?

kylebgorman commented 4 years ago

On Mon, Jan 13, 2020 at 10:29 AM Pierre-Andre Noel notifications@github.com wrote:

There may be a better alternative but I think it'll require a larger redesign.

Do you predict that type hinting and/or stubs https://docs.python.org/3/library/typing.html could help? There also appears to be some native Cython support http://docs.cython.org/en/latest/src/tutorial/pure.html#static-typing toward typing.

I'm still in the process of assessing if pywrapfst and/or pynini are appropriate for my use case but, if things continue the way they have, I may consider contributing some code toward such type hinting (which could be a good opportunity for me to learn Cython). Would you see value in that?

I would love more type hinting support and an unlikely to be able to do it myself.

I use hinting in all of my recent (pure) Python projects, and in teaching, and have found it useful.

You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/20?email_source=notifications&email_token=AABG4ON6Z3V36IRT64SDWS3Q5SCEZA5CNFSM4KE3YGP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIZEHHA#issuecomment-573719452, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4ONEXZNMO5NROIEA5GTQ5SCEZANCNFSM4KE3YGPQ .

kylebgorman commented 4 years ago

Hi @PierreAndreNoel , the next release of Pynini (2.1.0 maybe) will have the following change to the hierarchy:

One thing to understand about OpenFst is that most interesting FST classes are abstract. The only one you can construct as an empty FST is VectorFst; the others can only be created by reading them in, or by calling pywrapfst.convert on a VectorFst. This is documented in detail (and explicit in the code) at the C++ level for OpenFst so this change is just making this uncomfortable fact clearer from Python.

This of course is not to pre-empt your proposal to add some type-hinting.

kylebgorman commented 4 years ago

Not to re-open this but we also will have type stubs shipped with the next release, once we figure out how to package that.