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

[WIP] Add bytes strict type to parametrized functions #2837

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kramstrom
Copy link
Contributor

@kramstrom kramstrom commented Feb 5, 2025

Describe your changes

Do not merge -- need to merge backend support before this.

Adds support for new strict bytes type for modal.parameter

Usage:

@app.cls()
class C:
    a: bytes = modal.parameter(default=b"hello")
  • Provide Linear issue reference (e.g. MOD-1234) if available.
Backward/forward compatibility checks

Check these boxes or delete any item (or this section) if not relevant for this PR.

  • Client+Server: this change is compatible with old servers
  • Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.


Changelog

Adds support for new strict bytes type for modal.parameter

Usage:

import typing
import modal

app = modal.App()


@app.cls()
class Foo:
    a: bytes = modal.parameter(default=b"hello")

    @modal.method()
    def bar(self):
        return f"hello {self.a}"


@app.local_entrypoint()
def main():
    foo = Foo(a=b"world")
    foo.bar.remote()

For parameterized functions with web hooks you must base64 encode the bytes before passing them in as a parameter.

@kramstrom kramstrom changed the title Add bytes strict type to parametrized functions [WIP] Add bytes strict type to parametrized functions Feb 5, 2025
@kramstrom kramstrom requested a review from freider February 5, 2025 13:11
@kramstrom kramstrom marked this pull request as ready for review February 6, 2025 10:05
@kramstrom
Copy link
Contributor Author

Need to merge the protos before merging this & merge the server-side code before this

Copy link
Contributor

@freider freider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found a bug, and a matching bug in the test that makes it pass :D


@web_endpoint()
def web(self, arg):
return {"arg": arg, "bar": b"world"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should return self.bar right? :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants