Feature/Luma Ai Video Generation Driver #1199

william-price01 · 2024-09-23T20:08:25Z

I have read and agree to the contributing guidelines for submitting new pull requests.

Describe your changes

Added Luma AI as a driver, tool, task, to generate videos.

Issue ticket number and link

Add Luma Ai as a driver

…://github.com/griptape-ai/griptape into feature/dream_machine_video_generation_driver

codecov · 2024-09-25T18:09:24Z

Codecov Report

Attention: Patch coverage is 64.67662% with 71 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...eneration/dream_machine_video_generation_driver.py	44.44%	19 Missing and 1 partial ⚠️
griptape/loaders/video_loader.py	50.00%	14 Missing ⚠️
griptape/tasks/prompt_video_generation_task.py	60.71%	10 Missing and 1 partial ⚠️
...s/video_generation/base_video_generation_driver.py	57.14%	9 Missing ⚠️
griptape/mixins/artifact_file_output_mixin.py	11.11%	8 Missing ⚠️
griptape/tools/prompt_video_generation/tool.py	68.75%	5 Missing ⚠️
griptape/artifacts/video_artifact.py	83.33%	2 Missing ⚠️
griptape/tasks/base_video_generation_task.py	88.23%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

SavagePencil · 2024-09-25T21:29:18Z

griptape/artifacts/video_artifact.py

+        value: The video binary data.
+        mime_type: The video MIME type.
+        resolution: The resolution of the video (e.g., 1920x1080).
+        duration: Duration of the video in seconds.


which of these do you feel confident you can have in the initial version (for example, it sounded like a target duration may not be something easy to get)? Are there other params that the providers expose that we should be taking into consideration? For example, I would anticipate framerate to be a popular parameter so that we can have everyone's favorite NTSC 59.94!

I'm going to somehow include most of the things Jason mentioned, I don't believe that there is a way to bring audio with these videos, so probably no subtitles for initial version.

SavagePencil · 2024-09-25T21:30:05Z

griptape/artifacts/video_artifact.py

+
+    @property
+    def mime_type(self) -> str:
+        return "video/mp4"  # Or make this flexible based on the video format


sounds like our loaders support .ogg and .webm, can we point to the same loc for both or is there a reason to keep them separate?

We should already have the same mime-type umbrella problem for image/audio, if its not in the framework already, check with collin on the preferred approach.

Also, should this be an actual attribute instead of a @property?

@property is correct, just needs to be updated to something like this.

SavagePencil · 2024-09-25T21:31:21Z

griptape/artifacts/video_artifact.py

+        duration: Duration of the video in seconds.
+    """
+
+    aspect_ratio: tuple[int, int] = field(default=(16, 9), kw_only=True)


open question: do providers enumerate possible aspect ratios, or let you do weirdo things

SavagePencil · 2024-09-25T21:33:53Z

griptape/artifacts/video_artifact.py

+    def to_text(self) -> str:
+        raise NotImplementedError("VideoArtifact cannot be converted to text.")


other not-so-texty artifacts generate a string that describes the parameters of the artifact, is that what we want here, too?

This was more of a testing method, while getting it to actually work.

SavagePencil · 2024-09-25T21:35:26Z

griptape/drivers/__init__.py

+    "DreamMachineVideoGenerationDriver",
+    "BaseVideoGenerationDriver",


am I the only one at this damned company that likes my lists alphabetized? Not for this PR, but dang

I agree with james, but don't reorder this list in this PR. (Generally mixing refactoring or formatting with features makes it difficult for reviewers to see what is actually changing)

BaseVideoGenerationDriver should be placed before DreamMachineVideoGenerationDriver for circular dependency (and alphabetical) reasons.

SavagePencil · 2024-09-25T21:38:02Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+        response = self.client.generations.create(prompt=prompt, **self.params)
+        generation = response


instead of assigning to an alias, why not assign directly to generation?

SavagePencil · 2024-09-25T21:39:25Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+            if not generation.id:
+                raise Exception("Generation ID not found in the response")


Why is this in the while loop? Is this something that should be caught either before or after the dreaming begins?

Again, for testing purposes, pyright was giving me a hard time for not checking if the id exists.

SavagePencil · 2024-09-25T21:40:58Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+            if not video_url:
+                raise Exception("Video URL not found in the generation response")


If no URL shows up, is that indicative of something bad, like it "completed" but was somehow a failure? Can we convey what the situation is to the user?

SavagePencil · 2024-09-25T21:41:38Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+            video_url = generation.assets.video
+            if not video_url:
+                raise Exception("Video URL not found in the generation response")
+            video_binary = self._download_video(video_url)


downloading feels like it could wrong in a lot of other ways (retries, timeouts, etc.)

SavagePencil · 2024-09-25T21:42:45Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+        else:
+            raise Exception(f"Video generation failed with status: {status}")
+
+    def _download_video(self, video_url: str) -> bytes:


videos are big and take a long time to come down. Do we...

...already have this functionality somewhere else in the framework?

...need to take into consideration fails on memory, disk, retries, timeout?

id be fine with putting this functionality directly on VideoArtifact as a "lazy load" type feature. the alternative is creating a persistence driver for everything, which we dont have, but we have the ArtifactFileOutputMixin available.

Agreed that the Driver should probably not be the one to do this. I don't know about putting it in VideoArtifact either -- feels like too much responsibility.

In-fact, we probably shouldn't make any assumptions that the user wants us to download it. Maybe they're fine receiving a URL that they watch in their browser. Maybe this should fall onto a Loader?

could be the same idea as a lazy load. VideoArtifact contains a reference to the video somewhere.

vachillo · 2024-09-25T21:33:08Z

griptape/artifacts/video_artifact.py

+    def get_aspect_ratio(self) -> tuple[int, int]:
+        return self.aspect_ratio


dont need this, aspect_ratio can be directly accessed

vachillo · 2024-09-25T21:33:29Z

griptape/artifacts/video_artifact.py

+    def to_text(self) -> str:
+        raise NotImplementedError("VideoArtifact cannot be converted to text.")


use an approach similar to AudioArtifact for this

vachillo · 2024-09-25T21:34:30Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+    client: LumaAI = field(
+        default=Factory(
+            lambda self: import_optional_dependency("lumaai").LumaAI(auth_token=self.api_key), takes_self=True
+        ),
+        kw_only=True,
+    )


use the lazy_property approach for this

vachillo · 2024-09-25T21:34:53Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+        ),
+        kw_only=True,
+    )
+    params: dict[str, Any] = field(default={}, kw_only=True, metadata={"serializable": True})


use factory=dict here, this will return a single mutable object which is no good

This is important

vachillo · 2024-09-25T21:35:50Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+        while status in ["dreaming", "queued"]:
+            time.sleep(5)
+            if not generation.id:
+                raise Exception("Generation ID not found in the response")
+
+            generation = self.client.generations.get(generation.id)
+            status = generation.state


try to use the tenacity library for this here. i know this is the same approach as the cloud driver but we should try to clean this pattern up

I'm actually not sure about this. Retrying (tenacity) feels different than polling (what we're doing here).

tenacity has built in conditions for if_condition or if_not_condition. its not just for retrying on errors

I don't doubt it can be used for non-exception things, but all of the examples are for exception retrying which makes me think that's its primary purpose.

Do you have an example of how tenacity might be used for polling?

something like

from tenacity import retry, retry_if_result, stop_after_attempt, wait_fixed @retry( retry=retry_if_result(lambda result: result is None), stop=stop_after_attempt(3), wait=wait_fixed(5), ) def call_api(url_to_poll: str) -> Optional[str]: response = requests.get(url_to_poll) if response.status_code != 200: return None return response.text

vachillo · 2024-09-25T21:39:35Z

griptape/mixins/artifact_file_output_mixin.py

+    def save_video_artifact(self, artifact: VideoArtifact) -> None:
+        if self.output_file:
+            outfile = self.output_file
+        elif self.output_dir:
+            outfile = os.path.join(self.output_dir, artifact.name + ".mp4")
+        else:
+            raise ValueError("No output_file or output_dir specified.")


why this method? _write_to_file should be fine

For some reason, it would call the to_text method inside of the base artifact class which the Video Artifact is unable to be converted to text in the same way.

you need to override to_bytes on the video artifact

okay, that makes sense! Thank you!

vachillo · 2024-09-25T21:42:16Z

griptape/tasks/prompt_video_generation_task.py

+
+
+@define
+class PromptVideoGenerationTask(BaseVideoGenerationTask):


i dont think these need to be two different classes. the input can be different types instead

vachillo · 2024-09-25T21:43:07Z

griptape/tools/prompt_video_generation/tool.py

+            "description": "Generates a video from text prompts.",
+            "schema": Schema(
+                {
+                    Literal("prompt", description=BaseVideoGenerationTool.PROMPT_DESCRIPTION): str,


this can be inlined

vachillo · 2024-09-25T21:43:42Z

griptape/tools/prompt_video_generation/tool.py

+
+
+@define
+class PromptVideoGenerationTool(BaseVideoGenerationTool):


this doesnt need to be two classes, just inherit from BaseTool and make VideoGenerationTool

vachillo · 2024-09-25T21:44:37Z

griptape/tools/prompt_video_generation/tool.py

+            ),
+        },
+    )
+    def generate_video(self, params: dict[str, dict[str, str]]) -> VideoArtifact | ErrorArtifact:


this is just typed as params: dict

dylanholmes

Didn't quite get a full review in yet, but overall the approach seems fine. You are following existing patterns (e.g. image generation) which I like.

I'll try to review a little more later today

dylanholmes · 2024-09-26T12:40:50Z

griptape/artifacts/video_artifact.py

+
+    @property
+    def mime_type(self) -> str:
+        return "video/mp4"  # Or make this flexible based on the video format


We should already have the same mime-type umbrella problem for image/audio, if its not in the framework already, check with collin on the preferred approach.

Also, should this be an actual attribute instead of a @property?

dylanholmes · 2024-09-26T12:42:43Z

griptape/drivers/__init__.py

+    "DreamMachineVideoGenerationDriver",
+    "BaseVideoGenerationDriver",


I agree with james, but don't reorder this list in this PR. (Generally mixing refactoring or formatting with features makes it difficult for reviewers to see what is actually changing)

collindutter

Great start! Have not looked at the entire thing but left some initial feedback.

collindutter · 2024-09-26T16:41:00Z

griptape/artifacts/video_artifact.py

+
+    @property
+    def mime_type(self) -> str:
+        return "video/mp4"  # Or make this flexible based on the video format


Add a format field similar to AudioArtifact and use that here.

collindutter · 2024-09-26T16:42:01Z

griptape/artifacts/video_artifact.py

+
+    @property
+    def mime_type(self) -> str:
+        return "video/mp4"  # Or make this flexible based on the video format


@property is correct, just needs to be updated to something like this.

collindutter · 2024-09-26T16:43:23Z

griptape/drivers/__init__.py

+    "DreamMachineVideoGenerationDriver",
+    "BaseVideoGenerationDriver",


BaseVideoGenerationDriver should be placed before DreamMachineVideoGenerationDriver for circular dependency (and alphabetical) reasons.

collindutter · 2024-09-26T16:45:48Z

griptape/drivers/video_generation/base_video_generation_driver.py

+
+                return result
+        else:
+            raise Exception("Failed to run text to video generation")


This also caught my eye, but we do it in BasePromptDriver which is probably where this came from. RuntimeError would probably be a better candidate here.

collindutter · 2024-09-26T16:47:07Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+        generation = response
+        status = generation.state
+        while status in ["dreaming", "queued"]:
+            time.sleep(5)


Sleep time should be configurable as something like poll_interval

collindutter · 2024-09-26T16:49:52Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+                raise Exception("Generation ID not found in the response")
+
+            generation = self.client.generations.get(generation.id)
+            status = generation.state


Can get rid of status -- just use generation.state

collindutter · 2024-09-26T16:50:58Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+        while status in ["dreaming", "queued"]:
+            time.sleep(5)
+            if not generation.id:
+                raise Exception("Generation ID not found in the response")
+
+            generation = self.client.generations.get(generation.id)
+            status = generation.state


I'm actually not sure about this. Retrying (tenacity) feels different than polling (what we're doing here).

collindutter · 2024-09-26T16:51:28Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+
+            generation = self.client.generations.get(generation.id)
+            status = generation.state
+        if status == "completed":


Does the exa library provide any types for this so we can do something like status == exa.COMPLETED

collindutter · 2024-09-26T16:51:53Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+        if status == "completed":
+            video_url = generation.assets.video
+            if not video_url:
+                raise Exception("Video URL not found in the generation response")


Flip conditional as mentioned above. Also raise a ValueError

collindutter · 2024-09-26T16:55:37Z

griptape/drivers/video_generation/dream_machine_video_generation_driver.py

+        else:
+            raise Exception(f"Video generation failed with status: {status}")
+
+    def _download_video(self, video_url: str) -> bytes:


Agreed that the Driver should probably not be the one to do this. I don't know about putting it in VideoArtifact either -- feels like too much responsibility.

In-fact, we probably shouldn't make any assumptions that the user wants us to download it. Maybe they're fine receiving a URL that they watch in their browser. Maybe this should fall onto a Loader?

Finished Video Generation Driver

d45bb8e

william-price01 changed the title ~~Finished Video Generation Driver~~ Feature/Luma Ai Video Generation Driver Sep 24, 2024

william-price01 added 8 commits September 25, 2024 09:13

Merge branch 'dev' into feature/dream_machine_video_generation_driver

7481a8f

poetry lock --no-update

8084eab

poetry lock --no-update

1182b67

Merge branch 'dev' into feature/dream_machine_video_generation_driver

b3bce7f

poetry changes

2baaa01

Merge branch 'feature/dream_machine_video_generation_driver' of https…

113846a

…://github.com/griptape-ai/griptape into feature/dream_machine_video_generation_driver

poetry lock --no-update

1043ed2

Video Generation Tool

5156190

initial review

f3aa092

SavagePencil reviewed Sep 25, 2024

View reviewed changes

vachillo reviewed Sep 25, 2024

View reviewed changes

dylanholmes reviewed Sep 26, 2024

View reviewed changes

Merge branch 'dev' into feature/dream_machine_video_generation_driver

e65f31b

collindutter requested changes Sep 26, 2024

View reviewed changes

		def to_text(self) -> str:
		raise NotImplementedError("VideoArtifact cannot be converted to text.")

		"DreamMachineVideoGenerationDriver",
		"BaseVideoGenerationDriver",

		response = self.client.generations.create(prompt=prompt, **self.params)
		generation = response

		if not generation.id:
		raise Exception("Generation ID not found in the response")

		if not video_url:
		raise Exception("Video URL not found in the generation response")

		def get_aspect_ratio(self) -> tuple[int, int]:
		return self.aspect_ratio



		@define
		class PromptVideoGenerationTask(BaseVideoGenerationTask):



		@define
		class PromptVideoGenerationTool(BaseVideoGenerationTool):

Feature/Luma Ai Video Generation Driver #1199

Are you sure you want to change the base?

Feature/Luma Ai Video Generation Driver #1199

Conversation

william-price01 commented Sep 23, 2024 • edited Loading

Describe your changes

Issue ticket number and link

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

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

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

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

vachillo Sep 25, 2024 • edited Loading

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

dylanholmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collindutter 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

william-price01 commented Sep 23, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

vachillo Sep 25, 2024 •

edited

Loading