Skip to content

Add .comfyignore support to node packaging#321

Merged
bigcat88 merged 5 commits intoComfy-Org:mainfrom
HenkDz:main
Sep 30, 2025
Merged

Add .comfyignore support to node packaging#321
bigcat88 merged 5 commits intoComfy-Org:mainfrom
HenkDz:main

Conversation

@HenkDz
Copy link
Contributor

@HenkDz HenkDz commented Sep 27, 2025

Summary

  • teach node packaging commands to read an optional .comfyignore
  • ensure ignored files stay out of packed archives while required assets remain
  • document workflow so teammates get consistent bundles

Testing

  • pytest

- Introduced .comfyignore file to exclude files from packaging
- Updated zip_files function to respect .comfyignore patterns
- Added tests to validate .comfyignore functionality
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. documentation Improvements or additions to documentation enhancement New feature or request labels Sep 27, 2025
@bigcat88 bigcat88 self-requested a review September 27, 2025 17:50
@bigcat88
Copy link
Contributor

Hello. Can you please fix the linter error and failing test?

- Simplify type hints in file_utils.py by replacing List with built-in list
- Update file reading logic in _load_comfyignore_spec for clarity
- Introduce new test suite for file_utils, covering various scenarios
- Enhance requirements.compiled with additional comments for clarity
@HenkDz
Copy link
Contributor Author

HenkDz commented Sep 28, 2025

Hello. Can you please fix the linter error and failing test?

Done

- Enhance test_compile to filter out optional dependencies 🧪
@HenkDz
Copy link
Contributor Author

HenkDz commented Sep 28, 2025

Used Properly, the .comfyignore should really benefit the community, node devs can publish lightweight nodes while keeping all the dev noise aside without having to .gitignore any dev files.

@codecov
Copy link

codecov bot commented Sep 28, 2025

Codecov Report

❌ Patch coverage is 71.79487% with 22 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
comfy_cli/file_utils.py 71.79% 22 Missing ⚠️
@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   50.87%   52.00%   +1.12%     
==========================================
  Files          32       32              
  Lines        3444     3500      +56     
==========================================
+ Hits         1752     1820      +68     
+ Misses       1692     1680      -12     
Files with missing lines Coverage Δ
comfy_cli/command/custom_nodes/command.py 41.09% <ø> (ø)
comfy_cli/file_utils.py 78.26% <71.79%> (+23.02%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HenkDz
Copy link
Contributor Author

HenkDz commented Sep 28, 2025

@bigcat88 should I update the README ?

Copy link
Contributor

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

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

There is no need to update the main readme yet; the information in README_DEV is sufficient for now.


Should we update text here?

@app.command(
    "pack",
    help="Pack the current node into a zip file. Ignorining .gitignore files.",
)
@tracking.track_command("pack")

Is it possible to move test to the tests folder? (I can do it myself later in a separate PR)


I looked at the code, and overall I agree that it's a useful thing, and I can say in advance that we're ready to accept this PR after some minor clarifications.

- Add tests for zip_files function to ensure .comfyignore compliance 🧪
@HenkDz
Copy link
Contributor Author

HenkDz commented Sep 29, 2025

Thanks for the careful read, to answer your questions and inquiries I did the following:

• Updated the pack command help text so it now says “honoring .comfyignore patterns,” and cleaned up the typo.
• Relocated the new file-utils tests into the shared test suite as requested.
• The README note about forced files already matches the implementation: the packaging logic re-adds any paths listed in the include list even if they match .comfyignore, and the new test verifies that behavior.

Happy to expand or add more docs if you’d like!

@HenkDz
Copy link
Contributor Author

HenkDz commented Sep 29, 2025

Important to note:
Right now the pack command only “forces” files back into the archive if they’re listed in [tool.comfy.includes]. So if you add __init__.py to .comfyignore and don’t also list it under [includes], it gets skipped. The command doesn’t inject a default whitelist. That means the README line should be clarified (e.g., have folks add required files to [includes] or avoid ignoring them). If we want the CLI to always keep __init__.py (or other essentials), we’d need to add an explicit force-include list in code.

Copy link
Contributor

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed answers and for making this PR so easy to review. Everything looks great, and we are ready to merge it, if you are agree.

We plan to hold this in the main branch for about a week before cutting a new release. This will give users who build from source a chance to test it and ensure everything is stable.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 29, 2025
@HenkDz
Copy link
Contributor Author

HenkDz commented Sep 29, 2025

Most welcome, Sure go ahead and merge.
Really hope Node devs can see this as my custom_nodes folder is going out of hand eating the space, dev and production should be separate.

@bigcat88 bigcat88 merged commit 46501fa into Comfy-Org:main Sep 30, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants