pedroreyesalavrado commented on PR #8637: URL: https://github.com/apache/texera/pull/8637#issuecomment-5873813612
Thanks @Ma77Ball — all four actionable findings are addressed in e749a0c and 931a977. The fifth I did not do, and I say why below rather than leave it looking handled. **P1, ASF header.** You were right, and I verified it locally rather than from CI: of the 40 `.md` files tracked in the repo, 39 carry the header and the only one without it was mine. Worth noting the check never actually ran on this PR — the "Release Auditing" workflow is still pending maintainer approval for a first-time contributor, so only the seven lightweight checks executed. Your prediction just could not be observed yet. **P2, cross-operator state.** A real bug, and the omission was mine: I put the state in a root singleton deliberately (the panel destroys and rebuilds every frame on re-render) but never scoped it to an operator. The frame now combines the service state with its bound `operatorId` and renders idle when they differ, so another operator's suggestion is neither shown nor applicable. Four tests cover it, including the sequence you described and the case where the bound id changes under a live suggestion. **P2, demo-workflows/ folder.** Moved out of the repo root — but to `bin/single-node/examples/ai-fix-demos/`, beside `examples/workflows/` rather than inside it, which is a deliberate departure from your suggestion. `load-examples.sh` iterates `for workflow_file in "$WORKFLOW_DIR"/*.json`, so putting them in `workflows/` would create seven deliberately broken workflows in the account of anyone bringing the stack up with `--with-examples`. The glob is not recursive so a subdirectory would also work, but a sibling stays correct if it ever becomes recursive. The README explains this where the files live. Happy to drop the folder entirely if you would still rather not carry it. **P3, the bare `404`.** Fixed, and the false positive was real rather than theoretical — I added your cases as tests first and the suite went red on `File "udf.py", line 404, in process_tuple` classifying as `model_not_found`. The pattern now requires a model reference on the same line, which keeps the genuine shapes (`ModelNotFound`, `404 - model does not exist`, and HuggingFace's `404 Client Error: Not Found for url: .../models/...`) while dropping line numbers and unrelated HTTP statuses. **P3, model id from config — not done.** `GuiConfig` has no model field today; I checked the 20 keys `/api/config/gui` actually returns and there is nothing to read it from. Adding one is a backend change, which this PR deliberately avoids (no Scala, no new endpoints). I have named the coupling in a comment at the constant so the next person hits it with context instead of a bare 404, and I am happy to do it as a follow-up PR that adds the config field properly. Let me know if you would rather it block this one. Full suite: 209 passing. Thanks also @chenlica and @aglinxinyuan for weighing in on the direction. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
