Repository navigation
Avoid retaining dynamic workflow manifests in global caches - #3197
Conversation
|
👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely. 🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits. Warning 💸 The Claude reviewer bills in credits, not vibesAutomated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared. Draft when unsure, Ready when you mean it:
However you get there, arrive prepared:
Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.
|
|
🤖 Claude review started at commit New commits are not auto-reviewed. Add the |
Claude review summarySkills: review-workflows-execution-engine, review-topic-concurrency-and-resource-safety, review-topic-test-hygiene No high-confidence medium+ findings. What I checked in the code (I didn't rely on the PR description):
Minor doubt (non-blocking): during one compilation miss, Commands that informed the review: Reviewed at HEAD: f64500d |
Release coordination
Reviewed at HEAD: f64500d |
|
😎 PR passes the vibe-check and trust-me-bro verification. |
What improves
When a workflow with custom Python blocks has to be compiled again, it creates new Python classes describing those blocks. The compiled-workflow cache eventually evicts old workflows, but separate, process-wide metadata caches can still hold their generated classes alive. Repeated cache misses therefore retain old compilation objects and increase memory use.
#3196 restores reuse of compiled workflows for local custom Python blocks. This follow-up handles what happens when reuse is impossible or an old workflow is evicted: supporting caches no longer keep its generated manifests alive independently. Once the compiled workflow and other users release them, those classes become eligible for garbage collection.
The compiled-workflow cache still holds up to 256 workflows, and static built-in/plugin manifest caching stays enabled. On a compilation miss, dynamic manifest metadata is computed without inserting the generated class into a global supporting cache. A compilation-cache hit still reuses the compiled workflow.
flowchart TD A[HTTP workflow request] --> B{Compiled workflow cached?} B -->|Yes| C[Reuse compiled workflow] B -->|No| D[Generate custom block classes] D --> E[Read dynamic manifest metadata without globally caching the classes] E --> F[Compile workflow] F --> G[Store in existing compiled-workflow cache] G --> H[Run workflow] C --> H G --> I[Eventually evict old compiled workflow] I --> J[Release compilation-cache references] J --> K[Generated classes can be collected when no other owners remain] style E fill:#d9f2df,stroke:#27823b,color:#163d20The highlighted step is this PR's change. #3196 reduces how often classes are created; this PR prevents supporting caches from unnecessarily extending their lifetime. Garbage collection makes memory reusable, but Python/native allocators may retain freed pages, so RSS need not immediately fall.
Implementation
Performance observations and tradeoff
We did not observe an overall slowdown with this fix in the recorded experiments. The current implementation keeps the simpler approach: bypass supporting caches for dynamic manifests while preserving compiled-workflow reuse and static manifest caching.
The first two rows are earlier experiments comparing the original supporting-cache behavior against this fix; they are not measurements of #3196's cache-hit path. The last two rows compare #3196 alone against #3196 plus this fix. Times are elapsed workload times, not isolated per-function measurements.
* The combined run's 4.91 requests/s covers its recorded portion before the connection reset; the baseline completed the full phase. Both HTTP phases targeted 5 requests/s with at most four outstanding requests, so these rates do not establish maximum throughput. These were memory-focused experiments rather than repeated performance benchmarks: the observed results were comparable or faster with the fix, but they do not establish a reliable speedup or rule out overhead in other workloads.
The potential tradeoff is extra CPU work when the same dynamic manifest is inspected repeatedly:
We have not isolated the cost of those additional computations, so there is no measured percentage slowdown to quote. Reduced retention and garbage-collection costs could offset that work, but the experiments do not establish the cause of the observed timings. Class-owned metadata caching remains a possible future optimization if targeted profiling identifies a meaningful cost; it is not part of this PR.
T4 validation
A synthetic 1700×1028 image, public pretrained YOLO26 Medium model, and an eight-step workflow with three custom Python blocks exercised masking, inference, measurements and annotations over HTTP. External uploads were disabled. Each variant used a fresh process, a target of 5 requests/second and at most four outstanding requests. Both used the same existing GPU image and dependencies with mounted source: PR #3196 at
a7da6cafe7ba6d75d77e2307c81d3f1445dc4232, with or without this production patch.After 4,096 requests with one unchanged definition, the client cycled 512 equivalent definitions by changing only metadata. This exceeded the 256-entry compilation cache and forced repeated misses and eviction. No forced garbage collection, cache clearing or allocator trimming occurred during load.
The last panel measures the supporting manifest cache, not the compiled-workflow cache. The blue line has five static entries, which look almost like zero at this scale. Dynamic manifests still live inside cached compiled workflows.
The combined run's last 2,048 recorded requests stayed within 2.346–2.363 GiB. It encountered one connection reset near the end; the last persisted checkpoint was 8,064 of 8,192 planned eviction-phase responses. The server remained responsive and was not OOM-killed; the reset's cause is unproven. The comparison above uses the same completed-request checkpoint for both variants.
A separate fresh-process confirmation with connection reuse disabled completed 32 warm-up requests plus 4,096 forced misses, with no retries or transport errors. Final RSS was 2.362 GiB and peak RSS was 2.372 GiB. Post-load garbage collection left 768 live generated manifests = 256 cached workflows × 3 blocks, and only five static supporting-cache entries. This confirmation is separate from the plotted traces. Measurements and annotated-image bytes matched across completed phases; the interrupted phase's final image-signature aggregate was unavailable.
Checks
PYTHONPATH=workflows:inference_models .venv/bin/python -m pytest -q workflows/tests/unit_tests/execution_engine/introspection workflows/tests/unit_tests/execution_engine/compiler/test_dynamic_blocks_compilation_cache.py workflows/tests/unit_tests/execution_engine/compiler/test_cache.py workflows/tests/unit_tests/execution_engine/dynamic_blocs --disable-warnings.E9,F63,F7,F82) on the five production Python files and the two subsequently removed investigation files, andgit diff --check: passed.