Skip to content

Conversation

snomiao
Copy link
Member

@snomiao snomiao commented Oct 6, 2025

Summary

This PR refactors the GitHub Actions workflow structure to improve reusability, maintainability, and CI performance.

Changes

New Actions

  • setup-comfyui-server: New composite action that handles ComfyUI server setup and launch
    • Checks out ComfyUI repository
    • Installs ComfyUI_devtools custom node
    • Sets up Python environment and dependencies
    • Optionally launches the server with configurable parameters

Refactored Actions

  • setup-frontend: Simplified to focus only on frontend-specific tasks
    • Installs pnpm and Node.js
    • Installs dependencies
    • Optionally builds the frontend (can be skipped when using cached builds)
    • No longer handles server setup or checkout

Workflow Improvements

tests-ci.yaml

  • Introduced a setup job that builds once and caches the entire workspace
  • Test jobs now restore the cached workspace instead of rebuilding
  • Eliminated redundant setup steps in each test shard
  • Better separation between setup and test execution phases
  • Significant performance improvement through workspace caching

Locale Update Workflows

  • Updated update-locales.yaml to use the new action structure
  • Updated update-locales-for-given-custom-node-repository.yaml with proper custom node installation
  • Updated update-node-definitions-locales.yaml to use new actions
  • Removed working-directory references where appropriate

Other Workflows

  • Updated update-playwright-expectations.yaml to use new action structure
  • Consistent action usage across all workflows

Benefits

  1. Better Performance: Workspace caching eliminates redundant builds in CI, significantly reducing test execution time
  2. Improved Maintainability: Clear separation of concerns makes actions easier to understand and modify
  3. Enhanced Reusability: Actions can be composed in different ways for different workflows
  4. DRY Principle: Eliminated code duplication across workflows
  5. Easier Debugging: Smaller, focused actions make it easier to identify and fix issues

Testing

  • Verify tests-ci workflow runs successfully
  • Verify locale update workflows function correctly
  • Verify playwright expectations update workflow works
  • Confirm cache/restore mechanism works as expected

Related Issues

This refactoring addresses workflow complexity and reduces CI runtime by leveraging GitHub Actions caching more effectively.

┆Issue is synchronized with this Notion page by Unito

This refactoring improves the CI/CD workflow structure by:

- Split the monolithic setup-frontend action into two focused actions:
  - setup-frontend: Now only handles frontend dependency installation and building
  - setup-comfyui-server: New action for ComfyUI server setup and launch

- Simplified workflow structure with better separation of concerns:
  - Frontend and server setup are now independent and reusable
  - Each action has clearer responsibilities and inputs
  - Removed duplicate setup code across workflows

- Improved tests-ci.yaml workflow:
  - Uses cache/save and cache/restore for the entire workspace
  - Test jobs now restore cached build instead of rebuilding
  - Reduced redundant setup steps in each test shard
  - Better parallelization with faster test execution

- Updated all locale update workflows to use new action structure
- Made setup-playwright a standalone reusable action

Benefits:
- Faster CI runs by reducing redundant builds
- More maintainable with DRY principle
- Easier to debug individual components
- Better action reusability across workflows
Copy link

github-actions bot commented Oct 6, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 10/07/2025, 05:58:40 PM UTC

📈 Summary

  • Total Tests: 487
  • Passed: 453 ✅
  • Failed: 0
  • Flaky: 4 ⚠️
  • Skipped: 30 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 444 / ❌ 0 / ⚠️ 4 / ⏭️ 30
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

Copy link

github-actions bot commented Oct 6, 2025

🎨 Storybook Build Status

Build completed successfully!

⏰ Completed at: 10/07/2025, 05:34:48 PM UTC

🔗 Links


🎉 Your Storybook is ready for review!

path: |
./.cache
./tsconfig.tsbuildinfo
key: playwright-setup-cache-${{ runner.os }}-${{ hashFiles('./pnpm-lock.yaml') }}-${{ hashFiles('./src/**/*.{ts,vue,js}', './*.config.*') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This would invalidate practically every run, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I have to check what's included in .cache now

Copy link
Member Author

Choose a reason for hiding this comment

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

Migrated to frontend build action

The PLAYWRIGHT_BLOB_OUTPUT_DIR was set to ../blob-report which was correct
when working-directory was ComfyUI_frontend, but now that we removed the
working-directory, the path should be ./blob-report to output in the repo root.
@snomiao
Copy link
Member Author

snomiao commented Oct 6, 2025

Fix Applied

Fixed the failing job. The issue was with the blob report output path.

Problem

When removing the working-directory: ComfyUI_frontend from test jobs, the PLAYWRIGHT_BLOB_OUTPUT_DIR was still set to ../blob-report, which would try to write outside the repo root. This caused the blob report artifacts to be empty, leading to the merge-reports job failure.

Solution

Changed PLAYWRIGHT_BLOB_OUTPUT_DIR from ../blob-report to ./blob-report to output the reports in the repo root directory, where the artifact upload step expects them.

The workflow is now running again with this fix applied.

Add helpful error message when cp fails to copy devtools files.
This clarifies the requirement that the frontend repo must be
checked out before using this action.
Changes:
- Move cache restore to setup-frontend action BEFORE build operations
- Remove duplicate cache step from setup-playwright action
- Use cache/restore instead of cache to avoid auto-save behavior
- Rename cache key from 'playwright-setup-cache' to 'tool-cache' for clarity
- Include source file hashes in cache key for proper invalidation

Benefits:
- Cache is now restored before tools run, allowing them to use cached data
- Eliminates duplicate caching of ./.cache directory
- Cache properly invalidates when source files or configs change
- Follows GitHub Actions best practice of restore before, save after pattern
- The workspace cache in tests-ci.yaml handles saving the complete state

Note: The .cache directory contains outputs from ESLint, Prettier, Stylelint,
Knip, and TypeScript incremental builds. These should be restored before any
build/lint operations run.
@snomiao
Copy link
Member Author

snomiao commented Oct 7, 2025

Response to cache invalidation concern

I've refactored the .cache directory caching strategy to address this concern. Here's what changed:

Changes Made

  1. Moved cache to setup-frontend action - Now restored BEFORE build operations (not after)
  2. Removed from setup-playwright - Eliminated the duplicate cache step
  3. Better cache key - Now includes both pnpm-lock.yaml AND source file hashes:
    key: tool-cache-${{ runner.os }}-${{ hashFiles('./pnpm-lock.yaml') }}-${{ hashFiles('./src/**/*.{ts,vue,js,mts}', './*.config.*') }}

Why This Works Better

Cache invalidation is now appropriate:

  • Invalidates when dependencies change (pnpm-lock.yaml)
  • Invalidates when source files or configs change
  • But NOT on every run - only when actual code changes

What's cached:
The .cache directory contains incremental outputs from:

  • ESLint (.eslintcache)
  • Prettier (.prettierrcache)
  • Stylelint (.stylelintcache)
  • Knip cache
  • TypeScript (tsconfig.tsbuildinfo)

In tests-ci.yaml:
The entire workspace (including .cache) is saved after the setup job completes, so test shards can restore everything at once. This means:

  • Setup job: Restores tool cache, builds, then saves entire workspace
  • Test jobs: Restore entire workspace (includes the already-populated .cache)
  • No redundant individual cache saves needed

This approach is more efficient and follows GitHub Actions best practices for cache lifecycle management.

@snomiao snomiao requested a review from DrJKL October 7, 2025 17:32
@snomiao snomiao marked this pull request as ready for review October 7, 2025 17:32
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Oct 7, 2025
shell: bash
run: |
mkdir -p ComfyUI/custom_nodes/ComfyUI_devtools
if ! cp -r ./tools/devtools/* ComfyUI/custom_nodes/ComfyUI_devtools/; then
Copy link
Contributor

Choose a reason for hiding this comment

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

So this means the frontend isn't under ComfyUI_frontend/ComfyUI_frontend/ anymore? 😁
This makes me happy.

pip install wait-for-it
working-directory: ComfyUI

build_frontend: 'false'
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick check: When we invoke this, is build_frontend set to false or true more often?

If it's turned off explicitly more often than left on by default, can we flip the default to make building opt-in instead of opt-out?

I usually prefer being able to find the heavier uses by searching for when options are turned on.

@DrJKL
Copy link
Contributor

DrJKL commented Oct 7, 2025

Keep in mind, we've started moving some components of the codebase into packages, so not everything that impacts the build is under /src anymore
At that point, how much are we saving with the caching? How would you feel about removing that piece and seeing how it performs, then if it's a problem we can address it but with a baseline for comparison?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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