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

Split AppList into client and internal web RPCs #2073

Merged
merged 2 commits into from
Aug 6, 2024
Merged

Conversation

ekzhang
Copy link
Member

@ekzhang ekzhang commented Aug 5, 2024

As part of MOD-3370, I'm going to split out the RPCs for listing apps on the server, with the AppList RPC used by the client library's modal app list command.

Right now these are the same RPC, which is not great for either use case because:

  • The client gets returned extra data that isn't useful for it
  • The web server gets returned data that it doesn't use, and doesn't receive data that it needs, so the frontend needs to make 4+ additional requests for every app that it displays, which slows down the modal.com/apps page and creates unnecessary server load. It also means that we can't sort or filter the list by fields like number of calls, who deployed the app, or number of errors.

So we're splitting them out. The current AppList RPC will continue to be used so we keep compatibility with existing clients.

I'm removing these line of code and moving them to the server. There will be a brief interval where modal app list reports more recently-stopped apps than usual in the CLI while the server change is being rolled out:

        if (
            # Previously, all deployed objects (Dicts, Volumes, etc.) created an entry in the App table.
            # We are waiting to roll off support for old clients before we can clean up the database.
            # Until then, we filter deployed "single-object apps" from this output based on the object entity.
            (app_stats.object_entity and app_stats.object_entity != "ap")
            # AppList always returns up to the 250 most-recently stopped apps, which is a lot for the CLI
            # (it is also used in the web interface, where apps are organized by tabs and paginated).
            # So we semi-arbitrarily limit the stopped apps to those stopped within the past 2 hours.
            or (
                app_stats.state in {api_pb2.AppState.APP_STATE_STOPPED} and (now - app_stats.stopped_at) > (2 * 60 * 60)
            )
        ):

Tests

All tests pass except for version_test, which is an unrelated issue.

========================= 1 failed, 653 passed, 2 skipped, 2 warnings in 338.00s (0:05:37) ==========================

Server PR

corresponding server PR: https://github.com/modal-labs/modal/pull/14576

Changelog

  • Remove support for the undocumented modal.apps.list_apps() function, which was internal and not intended to be part of public API.

@ekzhang ekzhang requested a review from mwaskom August 5, 2024 17:55
Copy link
Contributor

@mwaskom mwaskom left a comment

Choose a reason for hiding this comment

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

Makes sense

@ekzhang
Copy link
Member Author

ekzhang commented Aug 5, 2024

@mwaskom how should we fix static type checks here?

@ekzhang
Copy link
Member Author

ekzhang commented Aug 5, 2024

Is from modal.app import list_apps a supported public API? It seems to leak Protobuf implementation details

@mwaskom
Copy link
Contributor

mwaskom commented Aug 5, 2024

Is from modal.app import list_apps a supported public API? It seems to leak Protobuf implementation details

I don't think we intend for it to be, although I'm sure someone has found it and used it...

We have an old ticket for improving the object-listing API: https://linear.app/modal-labs/issue/MOD-2779/add-an-api-method-to-list-named-objects

@ekzhang
Copy link
Member Author

ekzhang commented Aug 5, 2024

I removed modal.app.list_apps() to appease the static type translator, and since it's undocumented. Wrote a changelog entry since we were technically exposing it to users.

@ekzhang ekzhang merged commit 3c059b1 into main Aug 6, 2024
23 checks passed
@ekzhang ekzhang deleted the ekzhang/split-app-list branch August 6, 2024 03:56
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