Allow non-LLM recipes to run and move Data tab first in executions#4805
Conversation
…etrics Backend: inject stub model provider for sampler-only recipes so DataDesigner init does not reject empty provider lists. Frontend: use shared Spinner component, hide LLM columns metric and model usage card when recipe has no LLM columns.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
danielhanchen
left a comment
There was a problem hiding this comment.
Thank you for the PR! The goal of this PR is to allow non-LLM recipes (sampler/expression/validation-only) to run without a real model provider, and to improve the execution view UX by defaulting to the Data tab. As a summary, this PR removes the backend validation that blocked recipes without LLM columns, injects a stub provider to satisfy the DataDesigner registry, reorders the execution tabs so Data is first, adds a loading spinner while data is being generated, and conditionally hides LLM-specific metrics for non-LLM runs.
I reviewed this with 10 independent reviewers. The core changes are solid and well-structured. Two regressions were identified and I have pushed fixes directly:
| Reviewers | Severity | Finding |
|---|---|---|
| 6/10 | P2 | detailTab state was not reset when switching between executions -- the Data tab default only applied on first mount. Fixed by adding a useEffect keyed on selectedExecution?.id that resets the tab to data. |
| 1/10 | P2 | Terminal auto-scroll broke when the default tab moved from Overview to Data -- the scroll-to-bottom effect only ran on execution selection when the terminal DOM was absent. Fixed by adding detailTab to the effect deps so it also fires when the user opens the Overview tab. |
Other findings that were considered but determined to be either pre-existing issues or edge cases not worth blocking the PR on:
- Stub provider endpoint
http://localhost(3/10): The stub is never invoked for non-LLM recipes since DataDesigner only calls providers for LLM columns. - Spinner shows during
cancellingstatus (3/10): Thecancellingstate is very brief (seconds) so this is a minor cosmetic issue. - Provider gate only checks
llm-prefix (1/10): Pre-existing -- not introduced by this PR. - Progress accounting for multi-column non-LLM recipes (1/10): Pre-existing issue in
manager.py-- outside this PR's scope.
Concrete suggestions for the two fixes are in the commit I pushed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enables recipes without AI generation steps to run by introducing a stub model provider in the backend and conditionally hiding LLM-related metrics in the frontend. It also updates the execution view to default to the "Data" tab, adds a loading indicator for active executions, and reorders the detail tabs. Feedback was provided regarding the automatic reset of the active tab when switching between executions, as it may disrupt the user's workflow.
| useEffect(() => { | ||
| setDetailTab("data"); | ||
| }, [selectedExecution?.id]); |
There was a problem hiding this comment.
Resetting the detailTab to "data" every time the selectedExecution changes (via selectedExecution?.id) can be disruptive to the user workflow. If a user is browsing through multiple executions to compare specific information (e.g., checking the "Overview" for errors or "Raw" for specific metadata across different runs), they will be forced to manually switch back to their desired tab after every click in the sidebar.
Since the initial state is already set to "data" at line 62, the default view will correctly be the Data tab when the component first loads. Consider removing this useEffect to allow the user's tab selection to persist while they navigate through different executions.
The previous scroll effect ran on every tab switch, which could reset the user's manual scroll position if they scrolled up in the terminal and briefly switched tabs. Now the scroll-to-bottom and sticky-bottom reset only fires when navigating to the Overview tab.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enables the execution of recipes that do not contain AI generation steps by introducing a stub model provider in the backend and updating the frontend to conditionally display LLM-related metrics. Additionally, it improves the user interface by adding a loading spinner to the data tab during execution and changing the default detail tab to 'Data'. Feedback suggests removing the logic that automatically resets the active tab to 'Data' when switching between executions, as this behavior can be disruptive to users who wish to maintain their current view, such as terminal logs, while navigating between different execution records.
| useEffect(() => { | ||
| if (!terminalRef.current) { | ||
| setDetailTab("data"); | ||
| }, [selectedExecution?.id]); | ||
|
|
||
| useEffect(() => { | ||
| if (detailTab !== "overview" || !terminalRef.current) { | ||
| return; | ||
| } | ||
| shouldStickTerminalToBottomRef.current = true; | ||
| terminalRef.current.scrollTop = terminalRef.current.scrollHeight; | ||
| }, [selectedExecution?.id]); | ||
| }, [detailTab, selectedExecution?.id]); |
There was a problem hiding this comment.
Automatically resetting the detailTab to "data" every time the selectedExecution changes can be disruptive to the user experience. For example, if a user is intentionally monitoring terminal logs in the "Overview" tab and wants to click through multiple executions in the sidebar to compare them, they will be forced back to the "Data" tab on every selection.
Since the default state is already set to "data" on line 62, the view will correctly default to the Data tab when the component first loads. It is generally better to preserve the user's active tab selection as they navigate between different executions.
useEffect(() => {
if (detailTab !== "overview" || !terminalRef.current) {
return;
}
shouldStickTerminalToBottomRef.current = true;
terminalRef.current.scrollTop = terminalRef.current.scrollHeight;
}, [detailTab, selectedExecution?.id]);
The stub ModelProvider that satisfies the DataDesigner registry for non-LLM recipes should not carry a fake credential string. Using None avoids sending an Authorization header if the provider is ever inadvertently invoked.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enables the execution of recipes that do not contain AI generation steps by providing a stub model provider in the backend and conditionally hiding LLM-related metrics in the frontend. Additionally, it improves the user interface by adding a loading spinner to the data tab during execution and changing the default execution view tab to 'Data'. Feedback was provided regarding a new useEffect hook that resets the active tab to 'Data' whenever a different execution is selected, which may negatively impact the user experience by breaking tab persistence during navigation.
| useEffect(() => { | ||
| if (!terminalRef.current) { | ||
| setDetailTab("data"); | ||
| }, [selectedExecution?.id]); |
There was a problem hiding this comment.
This useEffect forces the active tab back to 'Data' every time a different execution is selected in the sidebar. This breaks tab persistence; for example, if a user is looking at the 'Raw' tab and clicks through different executions to compare their configurations, they will be forced back to the 'Data' tab on every click. Since the default tab is already set to 'Data' in the useState initialization (line 62), this effect should be removed to allow the user's tab selection to persist during navigation.
…nslothai#4805) * feat: allow non-LLM recipes to run without provider block * feat: reorder execution tabs and add generation-aware data tab empty state * fix: add accessibility attrs to data tab spinner and use literal ellipsis * fix(studio): use shared spinner, stub provider, and hide unused LLM metrics Backend: inject stub model provider for sampler-only recipes so DataDesigner init does not reject empty provider lists. Frontend: use shared Spinner component, hide LLM columns metric and model usage card when recipe has no LLM columns. * Fix tab reset and terminal auto-scroll regressions for PR unslothai#4805 Reset detailTab to "data" when switching between executions so the Data tab default is applied consistently, not only on first mount. Also add detailTab to the terminal scroll effect deps so auto-scroll-to-bottom fires when the user opens the Overview tab after landing on Data. * Guard terminal scroll reset to only fire on Overview tab The previous scroll effect ran on every tab switch, which could reset the user's manual scroll position if they scrolled up in the terminal and briefly switched tabs. Now the scroll-to-bottom and sticky-bottom reset only fires when navigating to the Overview tab. * Use None for stub provider api_key instead of literal string The stub ModelProvider that satisfies the DataDesigner registry for non-LLM recipes should not carry a fake credential string. Using None avoids sending an Authorization header if the provider is ever inadvertently invoked. --------- Co-authored-by: Daniel Han <danielhanchen@gmail.com>
Summary
Test plan
Tested all manually