Skip to content

Conversation

bigcat88
Copy link
Contributor

@bigcat88 bigcat88 commented Oct 4, 2025

  1. We can safely enable the import-outside-toplevel rule, since there is no reason for API nodes not to import all required modules at the top of the file. If there is ever a strong need to import something elsewhere, we can mark that line with # noqa.

  2. After enabling the inconsistent-return-statements rule, I reviewed the validate_input_media function. It sometimes returns None, but overall does nothing useful. It can return a string in case of failed validation, but its result is never checked anywhere. When testing the API, I found that rewriting it to raise an error on validation would break some existing workflows, because the Moonvalley API successfully processes videos with non-odd dimensions (so the check itself is invalid if we enforce it). I also could not find any cases where with_frame_conditioning is required - maybe it was intended for a future video2video implementation that does not yet exist. For now, I suggest removing this unused function completely. If we need this functionality later, we can re-implement it in validation_utils.py so it can benefit other nodes and be easy to find/test.

Test that moving import does not break Gemini node:

Screenshot From 2025-10-04 07-46-31

Test to prove that dimension check for Moonvalley node is incorrect if we enable it:

moonvalley.mp4

@bigcat88 bigcat88 requested a review from Kosinkadink as a code owner October 4, 2025 04:59
@bigcat88
Copy link
Contributor Author

bigcat88 commented Oct 4, 2025

+label: Core

@comfy-pr-bot comfy-pr-bot added the Core Core team dependency label Oct 4, 2025
Copy link
Collaborator

@Kosinkadink Kosinkadink left a comment

Choose a reason for hiding this comment

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

Gotcha, since you've tested the conditions that the checks were looking for, looks good. I'll merge.

@Kosinkadink Kosinkadink merged commit 22f99fb into comfyanonymous:master Oct 4, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Core Core team dependency

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants