[Application] Introducing application runtime phase I #5234

alonmr · 2024-03-04T09:04:45Z

Application Runtime

MLRun allows user to deploy models and expose them for serving purposes.

We currently lack a way to enable users deploy an application atop those models. e.g. have a model that differentiate between cat and a dog and expose it while having a UI that allows the user to upload image to invoke the model with a given input. To do so, we introduce a way to deploy web application given user container image, port, and a command to run the http server.

The runtime is based on top of Nuclio and will add the application as a side-car to a Nuclio function while the actual function is a reverse proxy to that application.
The reason we want to use Nuclio for this runtime is to leverage its features such as scaling, triggers, monitoring, API gateway, etc.

Phase I

This phase is about making it work.

Configure the side car application.
Add the reverse proxy to the side car.

Usage example

# Create an application runtime (with pre built image)
application = project.set_function(name="my-vizro-dashboard", kind="application", image="repo/my-wabapp:latest")

# Set the port the side-car listens on
application.set_internal_app_port(port=8080)

# Deploy
application.deploy()

Jira - https://iguazio.atlassian.net/browse/ML-4601

# Conflicts: # mlrun/run.py # mlrun/runtimes/__init__.py # mlrun/runtimes/nuclio/__init__.py

TomerShor

Looking really good overall!
some comments below

TomerShor · 2024-03-12T09:48:22Z

mlrun/runtimes/nuclio/function.py

+        if not name:
+            raise ValueError("Sidecar name must be specified")


Why not just enrich with f"{name}-sidecar" here?

TomerShor · 2024-03-12T09:56:40Z

tests/runtimes/test_application.py

+    assert fn.spec.image == "mlrun/mlrun"
+    assert fn.metadata.name == "application-test"
+    assert (
+        "Ly8gQ29weXJpZ2h0IDIwMjMgSWd1YXppbwovLwovLyBMaWN"


I assume this is base64 of the go reverse proxy func? perhaps add a comment on this

mlrun/projects/project.py

mlrun/runtimes/nuclio/application/application.py

theSaarco

Cool stuff. Have some comments and suggestions.

theSaarco · 2024-03-17T15:33:58Z

mlrun/run.py

+        filename = str(ApplicationRuntime.reverse_proxy_file_path)
+        handler = "Handler"


Suggested change

filename = str(ApplicationRuntime.reverse_proxy_file_path)

handler = "Handler"

filename, handler = ApplicationRuntime.get_filename_and_handler()

It's better to ask the ApplicationRuntime class what these are - add a class method to the class to return them. This way any change in these will be local to the runtime class, and will not require searching through the code to find the reference here.

theSaarco · 2024-03-17T15:36:29Z

mlrun/run.py

+    def resolve_nuclio_sub_kind(_kind):
+        _is_nuclio = _kind.startswith("nuclio")
+        _sub_kind = (
+            _kind[_kind.find(":") + 1 :] if _is_nuclio and ":" in _kind else None
+        )
+        if _kind == RuntimeKinds.serving:
+            _is_nuclio = True
+            _sub_kind = serving_subkind
+        elif _kind == RuntimeKinds.application:
+            _is_nuclio = True
+        return _is_nuclio, _sub_kind


Since we're making these changes - isn't it better to move this to the RuntimeKinds class? Like resolve_nuclio_runtime?

mlrun/runtimes/__init__.py

theSaarco · 2024-03-17T15:39:32Z

mlrun/runtimes/nuclio/application/__init__.py

@@ -0,0 +1,15 @@
+# Copyright 2023 Iguazio


Suggested change

# Copyright 2023 Iguazio

# Copyright 2024 Iguazio

😃

nitpicker 🐛 😄
needs to be changed in all files

theSaarco · 2024-03-17T15:43:01Z

mlrun/runtimes/nuclio/application/application.py

+    def __init__(self, spec=None, metadata=None):
+        super().__init__(metadata, spec)


It's terrible that the order of parameters is different between this class and its super(). How did that happen? As far as I can tell, the parent is KubeResource which also does the same reversing (there's RemoteRuntime which doesn't implement __init__). So, does this even work?

oh wow fixed. copied it from KubeResource and fixed that one as well.

theSaarco · 2024-03-17T15:56:37Z

mlrun/runtimes/nuclio/function.py

@@ -958,6 +957,38 @@ def invoke(
            data = json.loads(data)
        return data

+    def _with_sidecar(self, name: str, image: str, port: int = None):


Since this is now a general nuclio functionality, why not make this a public function, allowing users to add sidecars to nuclio functions not just in the application runtime case?

mlrun/runtimes/nuclio/function.py

theSaarco · 2024-03-17T16:01:01Z

mlrun/runtimes/nuclio/function.py

+            sidecar["image"] = image
+
+        if port:
+            sidecar["ports"] = [


It looks like the sidecar function in nuclio supports multiple ports. Why not expose it here, and allow sending a list of ports to this function? Maybe change port to be ports: Optional[Union[int, list[int]]] = None?

theSaarco · 2024-03-17T16:06:41Z

tests/runtimes/test_application.py

+        "Ly8gQ29weXJpZ2h0IDIwMjMgSWd1YXppbwovLwovLyBMaWN"
+        in fn.spec.build.functionSourceCode
+    )
+    assert fn.spec.function_handler == "reverse_proxy:Handler"


Once you add the file-path and handler as class methods for the app runtime, you can use them to construct this value, rather than use a fixed string.

theSaarco · 2024-03-17T16:09:04Z

mlrun/runtimes/nuclio/application/application.py

+
+class ApplicationRuntime(RemoteRuntime):
+    kind = "application"
+    reverse_proxy_file_path = pathlib.Path(__file__).parent / "reverse_proxy.go"


Out of curiosity - does nuclio understand that it needs to use Golang runtime based on the suffix of the file? There doesn't seem to be any indication in the code here about it.

nuclio-jupyter enriches the spec.runtime from the file extension

mlrun/runtimes/nuclio/application/application.py

mlrun/runtimes/nuclio/application/reverse_proxy.go

tests/runtimes/test_application.py

theSaarco

Looks good 👍 .

TomerShor

Good work mate :)

liranbg

minor

liranbg · 2024-03-19T21:00:36Z

setup.py

@@ -59,6 +59,7 @@ def version():
            "server",
        ]
    ),
+    package_data={"": ["*.go"]},


provide the path and dont take all go files

liranbg · 2024-03-19T21:01:48Z

mlrun/runtimes/nuclio/application/application.py

+
+        if self.status.container_image:
+            self.from_image(self.status.container_image)
+            self.spec.build.functionSourceCode = ""


Suggested change

self.spec.build.functionSourceCode = ""

# nuclio implementation detail - when providing the image and emptying out the source code,

# nuclio skips rebuilding the image and simply takes the prebuilt image

self.spec.build.functionSourceCode = ""

alonmr added 30 commits January 24, 2024 11:30

init

a837fe3

init2

0f06bea

.

1d446e6

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

3ae076f

more small stuff

0f159ce

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

78c362a

test-1

799c437

fix

a16fd8f

fix

8ea7c5a

.

0e0f501

ut

63741ab

fixes

7198e2b

.

b587e59

.

39a57b5

.

67dbd30

Merge remote-tracking branch 'origin/development' into ML-4601

19caea4

# Conflicts: # mlrun/run.py # mlrun/runtimes/__init__.py # mlrun/runtimes/nuclio/__init__.py

fixes

8f33e80

minor

3d6c76e

.

65b1146

fix

0508459

.

1d57b77

fix

4ebae85

.

4f10cf7

test

35baa3c

fixes

53af10b

copyrigt

ff10bb8

minor

1d3b944

copyright

b1c8280

clear image

1c034cc

address initial cr

4558065

alonmr added 7 commits March 4, 2024 14:25

fixes

b13d946

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

ae799aa

set function from image and ut fixes

9926f61

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

813b87c

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

31078ab

add ut and fix

b5ee5e9

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

b9e3e59

alonmr marked this pull request as ready for review March 12, 2024 09:06

alonmr requested review from theSaarco, TomerShor and liranbg March 12, 2024 09:28

TomerShor reviewed Mar 12, 2024

View reviewed changes

alonmr added 3 commits March 13, 2024 12:26

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

8b9249c

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

ec61cd3

cr

d048278

theSaarco requested changes Mar 17, 2024

View reviewed changes

alonmr added 6 commits March 17, 2024 22:26

partial saar review

05d46bd

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

9b08eac

.

15da1fd

pre deploy validation

b72c1e7

Merge branch 'development' of github.com:mlrun/mlrun into ML-4601

7e3df15

ensure reverse proxy

80a3665

TomerShor reviewed Mar 18, 2024

View reviewed changes

tomer cr

558db4f

theSaarco approved these changes Mar 19, 2024

View reviewed changes

TomerShor approved these changes Mar 19, 2024

View reviewed changes

liranbg reviewed Mar 19, 2024

View reviewed changes

liran comments

94ab40d

alonmr merged commit a10578e into mlrun:development Mar 20, 2024
10 checks passed

alonmr deleted the ML-4601 branch March 20, 2024 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Application] Introducing application runtime phase I #5234

[Application] Introducing application runtime phase I #5234

alonmr commented Mar 4, 2024 •

edited

TomerShor left a comment

TomerShor Mar 12, 2024

TomerShor Mar 12, 2024

theSaarco left a comment

theSaarco Mar 17, 2024

theSaarco Mar 17, 2024

theSaarco Mar 17, 2024

alonmr Mar 17, 2024

theSaarco Mar 17, 2024

alonmr Mar 17, 2024

theSaarco Mar 17, 2024

theSaarco Mar 17, 2024

theSaarco Mar 17, 2024

theSaarco Mar 17, 2024

alonmr Mar 18, 2024

theSaarco left a comment

TomerShor left a comment

liranbg left a comment

liranbg Mar 19, 2024

liranbg Mar 19, 2024

		if not name:
		raise ValueError("Sidecar name must be specified")

		filename = str(ApplicationRuntime.reverse_proxy_file_path)
		handler = "Handler"

	filename = str(ApplicationRuntime.reverse_proxy_file_path)
	handler = "Handler"
	filename, handler = ApplicationRuntime.get_filename_and_handler()

		def __init__(self, spec=None, metadata=None):
		super().__init__(metadata, spec)

[Application] Introducing application runtime phase I #5234

[Application] Introducing application runtime phase I #5234

Conversation

alonmr commented Mar 4, 2024 • edited

Application Runtime

Phase I

Usage example

TomerShor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theSaarco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theSaarco left a comment

Choose a reason for hiding this comment

TomerShor left a comment

Choose a reason for hiding this comment

liranbg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonmr commented Mar 4, 2024 •

edited