Raise exception if you put app.function on a class method

[erikbern]

Quick fix for something a user ran into. User had code like this:

import modal

app = modal.App()

class X:
    @app.function()
    def f(self):
        ...

The issue is that @app.function should be @method() and you need a @app.cls instead.

However this code currently fails with this very user-unfriendly error:

AttributeError: 'NoneType' object has no attribute '__name__'

After this PR, it outputs this instead:

The `@app.function` decorator cannot be used on class methods.
Please use `@app.cls` with `@modal.method` instead. Example:

@app.cls()
class MyClass:
    @method()
    def f(self, x):
        ...

EDIT: this ended up being a bit more work than anticipated. I moved some code out of function_utils.py into app.py instead since it felt very Modal-specific. Passes all integration tests so hopefully it works!

[erikbern]

will look at unit test failures later

[mwaskom]

Am I doing something wrong here?

(modal) [devbox ~/scratch]
$ git -C ~/client show -s
commit 987613bc34d42adccc90ad2bca186d63ca674f71 (HEAD -> erikbern/classmethod-error, origin/erikbern/classmethod-error)
Author: Erik Bernhardsson <mail@erikbern.com>
Date:   Mon Jul 8 14:26:07 2024 +0000

    Don't raise error for partial functions



(modal) [devbox ~/scratch]
$ cat function_on_method.py
import modal

app = modal.App()

@app.cls()
class MyClass:
    @app.function()
    def f(self):
        print("Done!")



(modal) [devbox ~/scratch]
$ modal run function_on_method.py
╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ /home/ubuntu/scratch/function_on_method.py:6 in <module>                                         │
│                                                                                                  │
│    5 @app.cls()                                                                                  │
│ ❱  6 class MyClass:                                                                              │
│    7 │   @app.function()                                                                         │
│                                                                                                  │
│ /home/ubuntu/scratch/function_on_method.py:7 in MyClass                                          │
│                                                                                                  │
│    6 class MyClass:                                                                              │
│ ❱  7 │   @app.function()                                                                         │
│    8 │   def f(self):                                                                            │
│                                                                                                  │
│ /home/ubuntu/client/modal/app.py:552 in wrapped                                                  │
│                                                                                                  │
│   551 │   │   │   else:                                                                          │
│ ❱ 552 │   │   │   │   info = FunctionInfo(f, serialized=serialized, name_override=name)          │
│   553 │   │   │   │   webhook_config = None                                                      │
│                                                                                                  │
│ /home/ubuntu/client/modal/_utils/function_utils.py:119 in __init__                               │
│                                                                                                  │
│   118 │   │   │   │   )                                                                          │
│ ❱ 119 │   │   │   self.function_name = f"{user_cls.__name__}.{f.__name__}"                       │
│   120 │   │   else:                                                                              │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
AttributeError: 'NoneType' object has no attribute '__name__'

[mwaskom]

Suggested change

	@method()
	@modal.method()

to be more consistent with the prose, and because I think this is a better default style for our docs.

[erikbern]

I think the docs mostly use @method() rather than @modal.method. No super strong feelings but I think consistency favors the former?

[mwaskom]

By consistency I meant that the text of the error message has

Please use @app.cls with @modal.method instead.

I agree we're not very consistent with the docs, and personally would prefer always using the full modal.<function> spelling. Otherwise it is annoying because you need to update both the code where the error was raised and also change your imports. Additionally, since we use such general names, we run the risk of cascading confusing errors if the user happened to have a variable named method or whatever in their script (this is riskiest with exit, which is a built-in function).

Note that the ambiguity would be eliminated if the partial function decorators were methods on App instead of standalone functions in the modal namespace :)

[erikbern]

Sorry @mwaskom I think something broke when I rebased this on latest main. I'll ping you once I fixed it and all tests are passing.

[mwaskom]

Related confusing error that I just ran into when I forgot @modal.method:

$ modal run build_method_subclass.py::Base.f
╭─ Error ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ <function Base.f at 0x7f37eb248180> is not a Modal entity (should be an App or Function)                                                   │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

[erikbern]

oh cool let's try to fix that one too

[mwaskom]

Nice!

[mwaskom]

Does this this error obviate the one we have here:

modal-client/modal/_utils/function_utils.py

Lines 110 to 114 in ded0d09

	raise InvalidError(
	f"Cannot wrap `{f.__qualname__}`:"
	" functions and classes used in Modal must be defined in global scope."
	" If trying to apply additional decorators, they may need to use `functools.wraps`."
	)

?

[erikbern]

No, the other one is for situations where the a function is defined in local scope, i.e. inside another function.

The one we're adding is for global scope but not top-level scope.

[erikbern]

Thanks @mwaskom !