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.

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

nuitka is incompatible with overrides #2048

Closed
bersbersbers opened this issue Feb 10, 2023 · 19 comments
Closed

nuitka is incompatible with overrides #2048

bersbersbers opened this issue Feb 10, 2023 · 19 comments
Assignees
Labels
Milestone

Comments

@bersbersbers
Copy link

bersbersbers commented Feb 10, 2023

(Cross-posted from mkorpela/overrides#111 since I have no idea which instance may be best able to fix it.)

This code runs find in python but not after building with nuitka 1.4.4 on Python 3.10:

"""bug.py"""
from overrides import override

class A:
    def hello(self) -> None:
        print("A")

class B(A):
    @override
    def hello(self) -> None:
        print("B")

b = B()
b.hello()
$ pip install nuitka overrides
$ python bug.py
B
$ python -m nuitka bug.py
...
Nuitka:INFO: Successfully created 'bug.bin'.
$ ./bug.bin
Traceback (most recent call last):
  File "/mnt/c/code/bug.py", line 8, in <module>
    class B(A):
  File "/mnt/c/code/bug.py", line 9, in B
    @override
  File "/home/bers/.local/lib/python3.10/site-packages/overrides/overrides.py", line 143, in override
    return _overrides(method, check_signature, check_at_runtime)
  File "/home/bers/.local/lib/python3.10/site-packages/overrides/overrides.py", line 172, in _overrides
    raise TypeError(f"{method.__qualname__}: No super class method found")
TypeError: B.hello: No super class method found
@kayhayen kayhayen added enhancement An improvement rather than a bug delayed This is waiting for something else to be done first. funding_wanted Will do it for customers: https://nuitka.net/pages/commercial.html labels Feb 10, 2023
@kayhayen kayhayen self-assigned this Feb 10, 2023
@kayhayen
Copy link
Member

It seems the overrides packages parses byte code, which is really hard when there is no such thing. I find it confusing why it would do that. At least in your example, that seems unnecessary. It appears it would allow to see assignments that will be done later.

Now it will be an interesting puzzle to solve this. Effectively, _get_base_class_names(frame) would have to be executed at compile time, and then at run time, where to code object, say qualname would have to be a key, into a dictionary with the identifiers.

However, outside of Nuitka commercial I do not see myself doing this for a while. With somebody willing to pay my time there, no problem, but right now there is more important stuff to do otherwise.

@bersbersbers
Copy link
Author

bersbersbers commented Feb 10, 2023

No problem at all! I can easily fallback by such a construct here:

if whatever:
    from overrides import override
else:
    F = TypeVar("F", bound=Callable)
    def override(method: F) -> F:
        return method

Is there an ideal way to check that my code has been/is being compiled by Nuitka? PyInstaller defines sys.frozen, for instance: https://pyinstaller.org/en/stable/runtime-information.html

@kayhayen
Copy link
Member

kayhayen commented Feb 10, 2023

This looks then more like something the anti-bloat engine could do instead. Just replace @override with empty string. Check out this: https://nuitka.net/posts/nuitka-package-config-kickoff.html

If that is functionality equivalent (I think doc strings will not be copied), then we can get away with that and call it a day. Will you create a PR?

@bersbersbers
Copy link
Author

bersbersbers commented Feb 10, 2023

This looks like a good solution, too - I'll keep that in mind.

In the meantime, I think I found a good fix to @override myself (compare mkorpela/overrides#112), so maybe it's not even required to add overrides to Nuitkas list of bad-behavers. I'll see if my proposed change can be considered for override.

@kayhayen
Copy link
Member

Yes, we can do that, or even replace the whole function. You can experiment with this:

- module-name: 'overrides'
  anti-bloat:
    - description: 'disable overrides function checking'
      replacements:
      change_function:
        'overrides': "'(lambda fn: fn)'"

I will try that later, just replaces the whole function with that lambda. We do similar for other software I took this from 'tensorflow.python.ops.distributions.distribution' configuration.

@kayhayen kayhayen added help wanted Please help with this, we think you can and removed delayed This is waiting for something else to be done first. funding_wanted Will do it for customers: https://nuitka.net/pages/commercial.html labels Feb 11, 2023
@kayhayen kayhayen added this to the 1.4 milestone Feb 11, 2023
@kayhayen
Copy link
Member

I feel that's probably already the solution and of course could be in a next hotfix once confirmed.

@bersbersbers
Copy link
Author

Alright, will check tomorrow. Not sure yet if we need to change overrides.override, overrides.overrides, or both. Compare mkorpela/overrides@5d67cab

@bersbersbers
Copy link
Author

Yes, we can do that, or even replace the whole function. You can experiment with this:

- module-name: 'overrides'
  anti-bloat:
    - description: 'disable overrides function checking'
      replacements:
      change_function:
        'overrides': "'(lambda fn: fn)'"

I may be using it wrong, but that does not seem to have any effect for me:

vi ~/.pyenv/versions/3.10.10/envs/myenv/lib/python3.10/site-packages/nuitka/plugins/standard/standard.nuitka-package.config.yml
image

(Also tried overrides)

@kayhayen
Copy link
Member

The use of screenshots clearly indicates that working with text is not your strong suit. Eventually I will try myself.

@kayhayen
Copy link
Member

I think you didn't include overrides in the compilation, when accelerated mode won't do unless told, other than say standalone mode, and then of course anti-bloat will not use it. Also the module name is overrides.overrides, so it would not have matched, and even then, it seems the argument names of the function are not changed by the lamdba. I got an error there because the arg name was not method about fn not being defined.

This is probably way too black magic currently.

@bersbersbers
Copy link
Author

I think you didn't include overrides in the compilation

I think I did, otherwise I wouldn't get raise TypeError(f"{method.__qualname__}: No super class method found"), I think.

Also the module name is overrides.overrides

Do you mean the decorator name? Actually, there is overrides.overrides as well as overrides.override, compare mkorpela/overrides#108 and mkorpela/overrides@5d67cab and #2048 (comment) (and I tried all combinations in the code and the yaml file).

This is probably way too black magic currently.

I agree, compare #2048 (comment)

@mkorpela
Copy link

mkorpela commented Feb 12, 2023

I'm not at all sure should I merge the PR to overrides mkorpela/overrides#112 it basically silently just fails the whole operation.

It seems the overrides packages parses byte code, which is really hard when there is no such thing. I find it confusing why it would do that.

Because it checks override during class loading and that is the place where the super class is found from.
How would that (checking method override) work in case of a pre-compiled language?

I know that Python 3.12 will have a override decorator in types https://peps.python.org/pep-0698/

@bersbersbers
Copy link
Author

I'm not at all sure should I merge the PR to overrides mkorpela/overrides#112 it basically silently just fails the whole operation.

I agree - would it help if we made the check more specific? I noticed that dis.get_instructions(frame.f_code) returns an empty iterator, but I assume one may go a bit further still to find a specific indication that there is no byte code.

How would that (checking method override) would work in case of a pre-compiled language?

No idea, unfortunately.

I know that Python 3.12 will have a override decorator in types https://peps.python.org/pep-0698/

Interesting, thank you! Maybe Nuitka can (has to?) consider that one as well, and in effect consider both overrides.override and the new typing.override in one.

@bersbersbers
Copy link
Author

bersbersbers commented Feb 12, 2023

For the record, it will be in Python 3.12 via python/cpython#101564. Until then it is also available via python/typing_extensions#78. Since that "official" decorator is defined to not have any runtime effects, I would guess Nuitka does not have to care about either of those.

Whether Nuitka will need to support overrides certainly depends on what @mkorpela's plans are. @mkorpela, will you keep maintaining it to offer runtime checks, or will you deprecate? To be honest, I will probably switch from overrides to typing(_extensions) once mypy supports it (python/mypy#14072), because it means one less dependency and I am happy with mypy reporting override errors.

@kayhayen
Copy link
Member

For whatever feature is added to Python, there is always a backport available, that Nuitka gets to support. I doubt it will be different in this case, otherwise the feature will be unavailable for use in practically all libraries.

@bersbersbers
Copy link
Author

bersbersbers commented Feb 12, 2023

For whatever feature is added to Python, there is always a backport available, that Nuitka gets to support.

I understand that.

I doubt it will be different in this case, otherwise the feature will be unavailable for use in practically all libraries.

Well, what is different in this case is that, as I wrote, @override is defined to not have runtime effects, so Nuitka would not need to support any. I just checked, and this code runs fine after compilation with Nuitka:

from typing_extensions import override

class BaseClass:
    def hello(self) -> None:
        print(self)

class Class(BaseClass):
    @override
    def hello(self) -> None:
        print(self)

c = Class()
c.hello()

@kayhayen kayhayen added factory For issues fixed in factory only and removed help wanted Please help with this, we think you can labels Feb 12, 2023
@kayhayen
Copy link
Member

That's good. For that overrides package there is a fix on factory branch now. The typing_extensions are also a backport, are they, or just another implementation of the same thing, that will be in typing in the future?

@bersbersbers
Copy link
Author

That's good. For that overrides package there is a fix on factory branch now.

Great!

The typing_extensions are also a backport, are they, or just another implementation of the same thing, that will be in typing in the future?

Not sure I understand the difference to be honest. I'd say both, I guess. Check these two links for current implementations:

https://github.com/python/cpython/pull/101564/files#diff-ddb987fca5f5df0c9a2f5521ed687919d70bb3d64eaeb8021f98833a2a716887R3458-R3491

https://github.com/python/typing_extensions/blob/e81cb8289874d6a6715675d77df88db033824e0d/src/typing_extensions.py#L2093-L2131

@kayhayen
Copy link
Member

I had to make a hotfix the other day, and the change is actually part of 1.4.6 because it's only config.

@kayhayen kayhayen added bug and removed factory For issues fixed in factory only enhancement An improvement rather than a bug labels Feb 13, 2023
@Nuitka Nuitka locked and limited conversation to collaborators Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants