Skip to content

Conversation

@milovate
Copy link
Contributor

@milovate milovate commented Oct 24, 2025

Q/A checklist

  • I have tested my UI changes on mobile and they look acceptable
  • I have tested changes to the workflows in both the API and the UI
  • I have done a code review of my changes and looked at each line of the diff + the references of each function I have changed
  • My changes have not increased the import time of the server
How to check import time?

time python -c 'import server'

You can visualize this using tuna:

python3 -X importtime -c 'import server' 2> out.log && tuna out.log

To measure import time for a specific library:

$ time python -c 'import pandas'

________________________________________________________
Executed in    1.15 secs    fish           external
   usr time    2.22 secs   86.00 micros    2.22 secs
   sys time    0.72 secs  613.00 micros    0.72 secs

To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:

def my_function():
    import pandas as pd
    ...

Legal Boilerplate

Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.


@coderabbitai
Copy link

coderabbitai bot commented Oct 24, 2025

📝 Walkthrough

Walkthrough

Adds a nullable IntegerChoices Category (Video=1, Audio=2) and a category IntegerField to VideoModelSpec with a migration; updates Django admin search_fields and fieldsets to include category; adds ffprobe_metadata(filename: str) -> dict helper; and implements extensive audio-generation features and UI changes in recipes/VideoGenPage.py (audio model selection, OpenAPI-driven dynamic input rendering, audio orchestration, result URL normalization, ffmpeg-based audio/video merging, and related helpers).

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Areas to pay extra attention:

  • Migration and model field addition: nullability, choices, and potential data/model compatibility.
  • Admin changes: correct field names and ordering in search_fields/fieldsets.
  • recipes/VideoGenPage.py: large, dense new logic (OpenAPI schema extraction, dynamic form rendering, network calls, ffmpeg/ffprobe usage, file I/O, merging uploads) — require security, error handling, and resource cleanup review.
  • daras_ai_v2.exceptions.ffprobe_metadata: subprocess invocation, JSON parsing, and error propagation.
  • Integration points between video and audio flows, and result normalization (get_url_from_result) for varied response shapes.

Suggested reviewers

  • devxpy

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "feat: add generate sound effect" is clearly related to the changeset, which introduces audio generation capabilities to the VideoGenPage component. The title accurately captures the primary user-facing feature being added—the ability to generate sound effects and audio. While the title could be more comprehensive (not mentioning the supporting model categorization system or architectural changes), it is specific enough and directly references a core aspect of the changes rather than being vague or misleading.
Description Check ✅ Passed The pull request description closely follows the required template structure and includes all major sections: the Q/A checklist with all four items present (three checked and one unchecked), the detailed "How to check import time?" section with bash commands and examples, and the complete legal boilerplate. The description is well-structured and adheres to the template format. The only notable observation is that the fourth checklist item about import time is left unchecked, which may warrant author verification given the raw summary shows new imports added to VideoGenPage.py (tempfile, os, requests, furl, etc.).
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch gen-audio-widget

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Nitpick comments (8)
daras_ai_v2/exceptions.py (1)

146-157: Add docstring/typing and consider unifying with ffprobe() to avoid duplication.

Minor polish: annotate return type precisely and document the helper. Optionally, consolidate both wrappers into one that accepts a show arg ("streams" vs "format") to reduce duplication.

-def ffprobe_metadata(filename: str) -> dict:
+def ffprobe_metadata(filename: str) -> dict[str, typing.Any]:
+    """Return ffprobe container-level metadata (format) as a parsed JSON dict."""
     text = call_cmd(
         "ffprobe",
         "-v",
         "quiet",
         "-print_format",
         "json",
         "-show_format",
         filename,
         err_msg=FFMPEG_ERR_MSG,
     )
     return json.loads(text)
ai_models/models.py (1)

4-7: Set a default and index for category to match query patterns.

You filter by category in hot paths; add db_index=True. To avoid future NULLs, consider default=Category.Video (once backfill is in place) and null=False.

-    category = models.IntegerField(
-        choices=Category.choices,
-        help_text="Model category: generates Audio, Video, etc.",
-        null=True,
-    )
+    category = models.IntegerField(
+        choices=Category.choices,
+        help_text="Model category: generates Audio, Video, etc.",
+        null=True,  # switch to False after backfill
+        db_index=True,
+        # default=Category.Video,  # enable after 0004 backfill
+    )

Also applies to: 25-30

recipes/VideoGenPage.py (5)

159-166: Handle missing x-fal-order-properties.

Fallback to schema properties if the ordering hint is absent.

-            fields = model_output_schemas[0].get("x-fal-order-properties")
+            schema0 = model_output_schemas[0]
+            fields = schema0.get("x-fal-order-properties") or list(
+                schema0.get("properties", {}).keys()
+            )

149-153: Preserve traceback when re-raising to aid debugging.

Use exception chaining and keep message concise.

-            except Exception as e:
-                logger.error(f"Error getting output fields: {e}")
-                raise RuntimeError(f"Error getting output fields: {e}")
+            except Exception as e:
+                logger.error(f"Error getting output fields: {e}")
+                raise RuntimeError("Error getting output fields") from e

112-117: Type hint of generator return is inaccurate.

This generator yields progress strings; update the annotation.

-    ) -> typing.Iterator[dict[str, str]]:
+    ) -> typing.Iterator[str]:

485-487: Audio form: fallback when order hint is missing.

Mirror the video form’s defensive logic.

-        ordered_fields = schema.get("x-fal-order-properties")
+        ordered_fields = schema.get("x-fal-order-properties") or list(
+            schema.get("properties", {}).keys()
+        )

478-481: Wrap schema extraction in try/except like the video form.

Prevents UI from breaking if OpenAPI is malformed.

-        model_input_schemas = get_input_fields(models)
-
-        if not model_input_schemas:
-            return
+        try:
+            model_input_schemas = get_input_fields(models)
+        except Exception as e:
+            logger.error(f"Error getting audio input fields: {e}")
+            return
+        if not model_input_schemas:
+            return
ai_models/admin.py (1)

19-24: Use list_filter for category; drop it from search_fields.

search_fields on an IntegerField can produce inefficient casts or DB errors. Filter by category instead.

     list_filter = [
-        "pricing__category",
+        "category",
+        "pricing__category",
         "pricing__provider",
         "created_at",
         "updated_at",
     ]
@@
-    search_fields = ["name", "label", "model_id", "category"] + [
+    search_fields = ["name", "label", "model_id"] + [
         f"pricing__{field}" for field in ModelPricingAdmin.search_fields
     ]

Also applies to: 26-28, 44-45

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f453108 and 3f7b94d.

📒 Files selected for processing (5)
  • ai_models/admin.py (2 hunks)
  • ai_models/migrations/0003_videomodelspec_category.py (1 hunks)
  • ai_models/models.py (2 hunks)
  • daras_ai_v2/exceptions.py (1 hunks)
  • recipes/VideoGenPage.py (10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ai_models/admin.py (1)
usage_costs/admin.py (1)
  • ModelPricingAdmin (62-80)
recipes/VideoGenPage.py (8)
ai_models/models.py (2)
  • VideoModelSpec (9-47)
  • Category (4-6)
daras_ai_v2/exceptions.py (4)
  • UserError (58-65)
  • ffmpeg (128-129)
  • ffprobe_metadata (146-157)
  • raise_for_status (19-49)
daras_ai_v2/fal_ai.py (1)
  • generate_on_fal (11-36)
daras_ai_v2/functional.py (1)
  • get_initializer (137-145)
daras_ai_v2/safety_checker.py (1)
  • safety_checker (13-19)
daras_ai_v2/variables_widget.py (1)
  • render_prompt_vars (322-334)
widgets/switch_with_section.py (1)
  • switch_with_section (5-36)
daras_ai/image_input.py (1)
  • upload_file_from_bytes (59-66)
🪛 Ruff (0.14.1)
ai_models/migrations/0003_videomodelspec_category.py

8-10: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


12-22: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

recipes/VideoGenPage.py

4-4: Redefinition of unused json from line 3

Remove definition: json

(F811)


18-18: Redefinition of unused CaseInsensitiveDict from line 17

Remove definition: CaseInsensitiveDict

(F811)


28-28: Redefinition of unused SAFETY_CHECKER_MSG from line 27

Remove definition: SAFETY_CHECKER_MSG

(F811)


28-28: Redefinition of unused safety_checker from line 27

Remove definition: safety_checker

(F811)


32-32: Redefinition of unused CaseInsensitiveDict from line 18

Remove definition: CaseInsensitiveDict

(F811)


150-150: Do not catch blind exception: Exception

(BLE001)


152-152: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


152-152: Avoid specifying long messages outside the exception class

(TRY003)


155-157: Avoid specifying long messages outside the exception class

(TRY003)


185-187: Avoid specifying long messages outside the exception class

(TRY003)


216-216: Probable use of requests call without timeout

(S113)


221-221: Probable use of requests call without timeout

(S113)


330-330: Redefinition of unused DeforumSDPage from line 328

Remove definition: DeforumSDPage

(F811)


331-331: Redefinition of unused LipsyncPage from line 329

Remove definition: LipsyncPage

(F811)


417-417: Do not catch blind exception: Exception

(BLE001)

Comment on lines +12 to +22
operations = [
migrations.AddField(
model_name="videomodelspec",
name="category",
field=models.IntegerField(
choices=[(1, "Video"), (2, "Audio")],
help_text="Model category: generates Audio, Video, etc.",
null=True,
),
),
]
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Backfill and index required to prevent empty model lists post-migration.

Adding a nullable category without a data migration will exclude all existing rows from UI queries that filter by category, breaking model selection until each row is edited. Add a RunPython migration to set existing rows to Video by default and consider indexing for frequent filtering.

Additional migration (new file example):

# ai_models/migrations/0004_backfill_videomodelspec_category.py
from django.db import migrations

def forwards(apps, schema_editor):
    VideoModelSpec = apps.get_model("ai_models", "VideoModelSpec")
    # Default all existing to Video (1). Adjust as needed.
    VideoModelSpec.objects.filter(category__isnull=True).update(category=1)

def backwards(apps, schema_editor):
    VideoModelSpec = apps.get_model("ai_models", "VideoModelSpec")
    VideoModelSpec.objects.filter(category=1).update(category=None)

class Migration(migrations.Migration):
    dependencies = [("ai_models", "0003_videomodelspec_category")]
    operations = [migrations.RunPython(forwards, backwards)]

If appropriate, follow up with:

migrations.AlterField(
    model_name="videomodelspec",
    name="category",
    field=models.IntegerField(choices=[(1, "Video"), (2, "Audio")], help_text=..., null=False, default=1, db_index=True),
)
🧰 Tools
🪛 Ruff (0.14.1)

12-22: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

🤖 Prompt for AI Agents
In ai_models/migrations/0003_videomodelspec_category.py lines 12-22, adding a
nullable category field without a backfill will make existing rows invisible to
category-filtering UI; add a follow-up RunPython migration that sets category=1
(Video) for all rows where category is NULL in the forwards function and
restores NULL in backwards, then optionally add a subsequent AlterField
migration to make category non-null with default=1 and db_index=True to enforce
the default and optimize queries.

Comment on lines +216 to +224
video_response = requests.get(video_url)
video_response.raise_for_status()
with open(video_path, "wb") as f:
f.write(video_response.content)

audio_response = requests.get(audio_url)
audio_response.raise_for_status()
with open(audio_path, "wb") as f:
f.write(audio_response.content)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Add HTTP timeouts to external downloads.

Avoid hanging request threads; optionally stream to reduce memory spikes.

-            video_response = requests.get(video_url)
+            video_response = requests.get(video_url, timeout=(5, 30))
             video_response.raise_for_status()
             with open(video_path, "wb") as f:
                 f.write(video_response.content)
 
-            audio_response = requests.get(audio_url)
+            audio_response = requests.get(audio_url, timeout=(5, 30))
             audio_response.raise_for_status()
             with open(audio_path, "wb") as f:
                 f.write(audio_response.content)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
video_response = requests.get(video_url)
video_response.raise_for_status()
with open(video_path, "wb") as f:
f.write(video_response.content)
audio_response = requests.get(audio_url)
audio_response.raise_for_status()
with open(audio_path, "wb") as f:
f.write(audio_response.content)
video_response = requests.get(video_url, timeout=(5, 30))
video_response.raise_for_status()
with open(video_path, "wb") as f:
f.write(video_response.content)
audio_response = requests.get(audio_url, timeout=(5, 30))
audio_response.raise_for_status()
with open(audio_path, "wb") as f:
f.write(audio_response.content)
🧰 Tools
🪛 Ruff (0.14.1)

216-216: Probable use of requests call without timeout

(S113)


221-221: Probable use of requests call without timeout

(S113)

🤖 Prompt for AI Agents
In recipes/VideoGenPage.py around lines 216 to 224, the HTTP GETs for video and
audio lack timeouts and streaming, which can hang and spike memory; update both
requests.get calls to include a sensible timeout (e.g., connect and read tuple)
and use stream=True, then write the response to file in chunked iterations
(e.g., response.iter_content with a small chunk size) while checking status via
raise_for_status before iterating; ensure the response is closed (use the with
context manager on the response object or call response.close()) to avoid leaks.

Comment on lines +256 to +259
{
model.name: model
for model in VideoModelSpec.objects.filter(category=Category.Video)
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Don’t drop existing models with NULL category during rollout.

Until backfill is complete, include NULL for video models so the page doesn’t render empty.

-        self.available_models = CaseInsensitiveDict(
-            {
-                model.name: model
-                for model in VideoModelSpec.objects.filter(category=Category.Video)
-            }
-        )
+        self.available_models = CaseInsensitiveDict(
+            {
+                model.name: model
+                for model in VideoModelSpec.objects.filter(
+                    Q(category=Category.Video) | Q(category__isnull=True)
+                )
+            }
+        )

Also applies to: 262-267

🤖 Prompt for AI Agents
In recipes/VideoGenPage.py around lines 256-259 and 262-267, the queryset
currently only selects models with category=Category.Video which will drop
models that have NULL category during rollout; change the filter to include NULL
categories as well (e.g., use category__in=[Category.Video, None] or a Q filter
combining category=Category.Video OR category__isnull=True) so models with NULL
category are preserved and the page doesn’t render empty.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (5)
recipes/VideoGenPage.py (5)

323-324: Remove duplicate imports.

DeforumSDPage and LipsyncPage are already imported on lines 321-322.

Apply this diff:

         from recipes.CompareText2Img import CompareText2ImgPage
         from recipes.DeforumSD import DeforumSDPage
         from recipes.Lipsync import LipsyncPage
-        from recipes.DeforumSD import DeforumSDPage
-        from recipes.Lipsync import LipsyncPage
         from recipes.VideoBots import VideoBotsPage

122-130: Guard against None before accessing audio_inputs.

request.audio_inputs can be None (per the type hint on line 48), so calling .get() directly will raise an AttributeError.

Apply this diff:

+            audio_inputs = request.audio_inputs or {}
             for key in ["prompt", "negative_prompt"]:
-                if not request.audio_inputs.get(key):
+                val = audio_inputs.get(key)
+                if not val:
                     continue
-
-                # # Safety check if not disabled
-                if self.request.user.disable_safety_checker:
+                if not self.request.user.disable_safety_checker:
-                    continue
-                yield "Running safety checker..."
-                safety_checker(text=request.audio_inputs[key])
+                    yield "Running safety checker..."
+                    safety_checker(text=val)

134-138: Cast duration to float and guard against None in audio_inputs merge.

ffprobe_metadata returns duration as a string. Also, request.audio_inputs may be None.

Apply this diff:

             model = VideoModelSpec.objects.get(name=request.selected_audio_model)
             raw_dur = ffprobe_metadata(video_url)["format"]["duration"]
-            payload = {"video_url": video_url, "duration": raw_dur}
-            payload = payload | request.audio_inputs
+            video_duration = float(raw_dur) if raw_dur not in (None, "N/A") else None
+            payload = {"video_url": video_url, "duration": video_duration}
+            payload |= (request.audio_inputs or {})
             res = yield from generate_on_fal(model.model_id, payload)

209-217: Add HTTP timeouts to prevent hanging.

Both requests.get calls lack timeouts, which can block request threads indefinitely. Optionally, use streaming to reduce memory usage for large files.

Apply this diff:

-            video_response = requests.get(video_url)
+            video_response = requests.get(video_url, timeout=(5, 30))
             video_response.raise_for_status()
             with open(video_path, "wb") as f:
                 f.write(video_response.content)
 
-            audio_response = requests.get(audio_url)
+            audio_response = requests.get(audio_url, timeout=(5, 30))
             audio_response.raise_for_status()
             with open(audio_path, "wb") as f:
                 f.write(audio_response.content)

248-260: Include models with NULL category during migration rollout.

Until all existing models have their category field backfilled, filtering only by category=Category.Video will exclude models where category is NULL, potentially rendering the page empty.

Apply this diff:

         self.available_models = CaseInsensitiveDict(
             {
                 model.name: model
-                for model in VideoModelSpec.objects.filter(category=Category.Video)
+                for model in VideoModelSpec.objects.filter(
+                    Q(category=Category.Video) | Q(category__isnull=True)
+                )
             }
         )
 
         self.available_audio_models = CaseInsensitiveDict(
             {
                 model.name: model
-                for model in VideoModelSpec.objects.filter(category=Category.Audio)
+                for model in VideoModelSpec.objects.filter(
+                    Q(category=Category.Audio)
+                )
             }
         )
🧹 Nitpick comments (3)
recipes/VideoGenPage.py (3)

140-150: Refine exception handling to preserve context.

Consider using raise from e to maintain the exception chain, and narrow the exception type if possible.

Apply this diff:

             model_output_schemas = []
             try:
                 model_output_schemas = get_output_fields([model])
-            except Exception as e:
+            except (KeyError, ValueError, TypeError) as e:
                 logger.error(f"Error getting output fields: {e}")
-                raise RuntimeError(f"Error getting output fields: {e}")
+                raise RuntimeError(f"Error getting output fields: {e}") from e
 
             if not model_output_schemas:
                 raise RuntimeError(
                     f"No output fields returned from {request.selected_audio_model}"
                 )

152-180: Consider more robust field type detection.

The current string-matching approach ("video" in field, "audio" in field) is fragile. Consider checking the schema's type information or using explicit field names from the model specification.

Additionally, the warning log on line 175 is immediately followed by raising an exception, making the warning redundant:

                 else:
-                    logger.warning(
-                        f"Unknown field returned from {request.selected_audio_model}: {field.get('name')}"
-                    )
                     raise RuntimeError(
                         f"Unknown field returned from {request.selected_audio_model}"
                     )

407-412: Consider showing error to user instead of silent failure.

When get_input_fields fails, the form silently fails to render (early return). Consider displaying an error message to the user using gui.error() or narrowing the exception type.

     model_input_schemas = []
     try:
         model_input_schemas = get_input_fields(models)
-    except Exception as e:
+    except (KeyError, ValueError, TypeError) as e:
         logger.error(f"Error getting input fields: {e}")
+        gui.error(f"Error loading model input fields: {e}")
         return
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f7b94d and 7adcb61.

📒 Files selected for processing (1)
  • recipes/VideoGenPage.py (10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
recipes/VideoGenPage.py (7)
ai_models/models.py (2)
  • VideoModelSpec (9-47)
  • Category (4-6)
daras_ai_v2/exceptions.py (4)
  • UserError (58-65)
  • ffmpeg (128-129)
  • ffprobe_metadata (146-157)
  • raise_for_status (19-49)
daras_ai_v2/fal_ai.py (1)
  • generate_on_fal (11-36)
daras_ai_v2/functional.py (1)
  • get_initializer (137-145)
daras_ai_v2/safety_checker.py (1)
  • safety_checker (13-19)
widgets/switch_with_section.py (1)
  • switch_with_section (5-36)
daras_ai/image_input.py (1)
  • upload_file_from_bytes (59-66)
🪛 Ruff (0.14.1)
recipes/VideoGenPage.py

143-143: Do not catch blind exception: Exception

(BLE001)


145-145: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


145-145: Avoid specifying long messages outside the exception class

(TRY003)


148-150: Avoid specifying long messages outside the exception class

(TRY003)


178-180: Avoid specifying long messages outside the exception class

(TRY003)


209-209: Probable use of requests call without timeout

(S113)


214-214: Probable use of requests call without timeout

(S113)


323-323: Redefinition of unused DeforumSDPage from line 321

Remove definition: DeforumSDPage

(F811)


324-324: Redefinition of unused LipsyncPage from line 322

Remove definition: LipsyncPage

(F811)


410-410: Do not catch blind exception: Exception

(BLE001)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (3)
recipes/VideoGenPage.py (3)

607-640: LGTM!

The OpenAPI schema extraction logic is well-structured with safe dictionary traversal using .get() with defaults. The function properly handles both input and output schema types and gracefully returns None when schemas are missing.


184-199: LGTM!

The result normalization logic cleanly handles multiple result formats (None, list, dict, string) and provides a consistent URL output.


219-245: LGTM!

The FFmpeg command is well-constructed with explicit stream mapping and the -shortest flag to prevent duration mismatches. The merged video is properly uploaded to GCS with the correct content type.

@milovate milovate requested a review from devxpy October 25, 2025 01:51
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (7)
recipes/VideoGenPage.py (7)

323-324: Remove duplicate imports that are still present.

Despite being marked as addressed, these duplicate imports of DeforumSDPage and LipsyncPage (already imported at lines 321-322) remain in the code and will cause redefinition errors.

Apply this diff:

         from recipes.CompareText2Img import CompareText2ImgPage
         from recipes.DeforumSD import DeforumSDPage
         from recipes.Lipsync import LipsyncPage
-        from recipes.DeforumSD import DeforumSDPage
-        from recipes.Lipsync import LipsyncPage
         from recipes.VideoBots import VideoBotsPage

122-131: Fix NPE on audio_inputs - unresolved from previous review.

request.audio_inputs can be None, causing an AttributeError when calling .get(key). Additionally, the safety checker is passed the wrong value on line 130.

Apply this diff:

+            audio_inputs = request.audio_inputs or {}
             for key in ["prompt", "negative_prompt"]:
-                if not request.audio_inputs.get(key):
+                val = audio_inputs.get(key)
+                if not val:
                     continue
-
-                # # Safety check if not disabled
-                if self.request.user.disable_safety_checker:
+                if not self.request.user.disable_safety_checker:
                     continue
                 yield "Running safety checker..."
-                safety_checker(text=request.audio_inputs[key])
+                safety_checker(text=val)

134-138: Cast duration to float and guard against None audio_inputs - unresolved from previous review.

ffprobe_metadata returns duration as a string, and request.audio_inputs may be None, causing a TypeError when merging dictionaries.

Apply this diff:

             model = VideoModelSpec.objects.get(name=request.selected_audio_model)
             raw_dur = ffprobe_metadata(video_url)["format"]["duration"]
-            payload = {"video_url": video_url, "duration": raw_dur}
-            payload = payload | request.audio_inputs
+            video_duration = float(raw_dur) if raw_dur not in (None, "N/A") else None
+            payload = {"video_url": video_url, "duration": video_duration}
+            payload |= (request.audio_inputs or {})

209-217: Add HTTP timeouts to external downloads - unresolved from previous review.

Missing timeouts can hang request threads. Add timeout parameter and optionally stream to reduce memory usage for large files.

Apply this diff:

-            video_response = requests.get(video_url)
+            video_response = requests.get(video_url, timeout=(5, 30))
             video_response.raise_for_status()
             with open(video_path, "wb") as f:
                 f.write(video_response.content)
 
-            audio_response = requests.get(audio_url)
+            audio_response = requests.get(audio_url, timeout=(5, 30))
             audio_response.raise_for_status()
             with open(audio_path, "wb") as f:
                 f.write(audio_response.content)

248-260: Include NULL category models during rollout - unresolved from previous review.

Until all models have a category assigned, excluding NULL values will drop existing video models and potentially render the page empty.

Apply this diff:

         self.available_models = CaseInsensitiveDict(
             {
                 model.name: model
-                for model in VideoModelSpec.objects.filter(category=Category.Video)
+                for model in VideoModelSpec.objects.filter(
+                    Q(category=Category.Video) | Q(category__isnull=True)
+                )
             }
         )
 
         self.available_audio_models = CaseInsensitiveDict(
             {
                 model.name: model
                 for model in VideoModelSpec.objects.filter(category=Category.Audio)
             }
         )

480-483: Guard against None before calling sort - unresolved from previous review.

If x-fal-order-properties is absent in the schema, ordered_fields will be None, causing an AttributeError on line 483.

Apply this diff:

         schema = model_input_schemas[0]
         required_fields = set(schema.get("required", []))
-        ordered_fields = schema.get("x-fal-order-properties")
+        ordered_fields = schema.get("x-fal-order-properties") or []
         ordered_fields.sort(key=lambda x: x not in required_fields)

152-153: Guard against None fields before iteration.

If x-fal-order-properties is missing from the schema, fields will be None, causing a TypeError when attempting to iterate.

Apply this diff:

-            fields = model_output_schemas[0].get("x-fal-order-properties")
+            fields = model_output_schemas[0].get("x-fal-order-properties") or []
+            if not fields:
+                raise RuntimeError(
+                    f"No output field order found for {request.selected_audio_model}"
+                )
             for field in fields:
🧹 Nitpick comments (1)
recipes/VideoGenPage.py (1)

140-150: Improve exception handling specificity.

Catching bare Exception and raising without chaining obscures the root cause. Consider catching specific exceptions (e.g., KeyError, IndexError, requests.RequestException) and using raise ... from e to preserve the traceback.

Apply this diff:

             model_output_schemas = []
             try:
                 model_output_schemas = get_output_fields([model])
-            except Exception as e:
+            except (KeyError, ValueError, TypeError) as e:
                 logger.error(f"Error getting output fields: {e}")
-                raise RuntimeError(f"Error getting output fields: {e}")
+                raise RuntimeError(f"Error getting output fields for {model.name}") from e
 
             if not model_output_schemas:
                 raise RuntimeError(
-                    f"No output fields returned from {request.selected_audio_model}"
+                    f"No output fields found for {request.selected_audio_model}"
                 )
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7adcb61 and efefa22.

📒 Files selected for processing (1)
  • recipes/VideoGenPage.py (10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
recipes/VideoGenPage.py (6)
ai_models/models.py (2)
  • VideoModelSpec (9-47)
  • Category (4-6)
daras_ai_v2/exceptions.py (4)
  • UserError (58-65)
  • ffmpeg (128-129)
  • ffprobe_metadata (146-157)
  • raise_for_status (19-49)
daras_ai_v2/fal_ai.py (1)
  • generate_on_fal (11-36)
daras_ai_v2/safety_checker.py (1)
  • safety_checker (13-19)
widgets/switch_with_section.py (1)
  • switch_with_section (5-36)
daras_ai/image_input.py (1)
  • upload_file_from_bytes (59-66)
🪛 Ruff (0.14.1)
recipes/VideoGenPage.py

143-143: Do not catch blind exception: Exception

(BLE001)


145-145: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


145-145: Avoid specifying long messages outside the exception class

(TRY003)


148-150: Avoid specifying long messages outside the exception class

(TRY003)


178-180: Avoid specifying long messages outside the exception class

(TRY003)


209-209: Probable use of requests call without timeout

(S113)


214-214: Probable use of requests call without timeout

(S113)


323-323: Redefinition of unused DeforumSDPage from line 321

Remove definition: DeforumSDPage

(F811)


324-324: Redefinition of unused LipsyncPage from line 322

Remove definition: LipsyncPage

(F811)


410-410: Do not catch blind exception: Exception

(BLE001)


475-475: Do not catch blind exception: Exception

(BLE001)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test (3.10.12, 1.8.3)

Comment on lines +152 to +180
fields = model_output_schemas[0].get("x-fal-order-properties")
for field in fields:
if "video" in field:
video_result = res.get("video")
video_url = self.get_url_from_result(video_result)
if video_url:
merged_videos[video_model_name] = video_url
break
elif "audio" in field:
audio_result = res.get("audio")
audio_url = self.get_url_from_result(audio_result)
filename = (
furl(video_url.strip("/")).path.segments[-1].rsplit(".", 1)[0]
+ "_gooey_ai_"
+ request.selected_audio_model
+ ".mp4"
)
if audio_url:
merged_videos[video_model_name] = self.merge_audio_and_video(
filename, audio_url, video_url
)
break
else:
logger.warning(
f"Unknown field returned from {request.selected_audio_model}: {field.get('name')}"
)
raise RuntimeError(
f"Unknown field returned from {request.selected_audio_model}"
)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix type confusion in field iteration.

Line 154 checks if "video" in field assuming field is a string, but line 176 calls field.get('name') treating it as a dict. The x-fal-order-properties likely returns field names as strings, not dicts.

Apply this diff to fix the logic:

             fields = model_output_schemas[0].get("x-fal-order-properties")
             for field in fields:
-                if "video" in field:
+                if "video" in field.lower():
                     video_result = res.get("video")
                     video_url = self.get_url_from_result(video_result)
                     if video_url:
                         merged_videos[video_model_name] = video_url
                     break
-                elif "audio" in field:
+                elif "audio" in field.lower():
                     audio_result = res.get("audio")
                     audio_url = self.get_url_from_result(audio_result)
                     filename = (
                         furl(video_url.strip("/")).path.segments[-1].rsplit(".", 1)[0]
                         + "_gooey_ai_"
                         + request.selected_audio_model
                         + ".mp4"
                     )
                     if audio_url:
                         merged_videos[video_model_name] = self.merge_audio_and_video(
                             filename, audio_url, video_url
                         )
                     break
                 else:
                     logger.warning(
-                        f"Unknown field returned from {request.selected_audio_model}: {field.get('name')}"
+                        f"Unknown field returned from {request.selected_audio_model}: {field}"
                     )
                     raise RuntimeError(
                         f"Unknown field returned from {request.selected_audio_model}"
                     )
🧰 Tools
🪛 Ruff (0.14.1)

178-180: Avoid specifying long messages outside the exception class

(TRY003)

@milovate milovate marked this pull request as ready for review October 25, 2025 01:58
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