-
-
Notifications
You must be signed in to change notification settings - Fork 470
Fixed run button state reset on route change and state stale issue when a model fails to load #859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
89ab10a
1212164
5715b60
21606dc
5682cbf
3781b62
b36e867
67cd9b3
1c6a638
ecb88c2
90cf407
4845123
827d47c
8d8e324
13d364a
82d2b45
fca0049
9fce33b
564d51a
56398a2
fa0e706
f737dd6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,12 +42,173 @@ export default function RunModelButton({ | |
| setLogsDrawerOpen = null, | ||
| }) { | ||
| const [jobId, setJobId] = useState(null); | ||
| const storageKey = experimentInfo?.id | ||
| ? `RUN_MODEL_JOB.${experimentInfo.id}` | ||
| : null; | ||
| const [showRunSettings, setShowRunSettings] = useState(false); | ||
| const [inferenceSettings, setInferenceSettings] = useState({ | ||
| inferenceEngine: null, | ||
| inferenceEngineFriendlyName: '', | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!storageKey || !window.storage) return; | ||
| (async () => { | ||
| try { | ||
| const stored = await window.storage.get(storageKey); | ||
| if (stored !== undefined && stored !== null) { | ||
| setJobId(stored); | ||
| } | ||
| } catch (e) { | ||
| // ignore storage errors | ||
| } | ||
| })(); | ||
| }, [storageKey]); | ||
|
|
||
| useEffect(() => { | ||
| if (!storageKey || !window.storage) return; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is so much new code for this one condition! I feel confident there must be a way to simplify/refactor this so it's not so complex? I feel we've already made it too complex and that's why we are having these bugs in the first place. Let me take a closer look when I'm not on a plane and see if I can make some suggestions. |
||
| if (!experimentInfo?.id) return; | ||
|
|
||
| let cancelled = false; | ||
| let intervalId: number | null = null; | ||
|
|
||
| const pollJob = async () => { | ||
| if (jobId === null) return; | ||
|
|
||
| let currentJobId = jobId; | ||
| if (jobId === -1) { | ||
| try { | ||
| if (storageKey && window.storage) { | ||
| const stored = await window.storage.get(storageKey); | ||
| if (stored === null || stored === undefined) { | ||
| if (!cancelled) setJobId(null); | ||
| return; | ||
| } | ||
| if (stored !== -1) { | ||
| currentJobId = stored; | ||
| if (!cancelled) setJobId(stored); | ||
| } else { | ||
| try { | ||
| const listRes = await fetch( | ||
| getAPIFullPath('jobs', ['list'], { | ||
| experimentId: experimentInfo.id, | ||
| }), | ||
| { method: 'GET' }, | ||
| ); | ||
| if (listRes.ok) { | ||
| const jobsList = await listRes.json(); | ||
| const recentFailure = Array.isArray(jobsList) | ||
| ? jobsList.find((j) => | ||
| ['FAILED', 'ERROR', 'STOPPED', 'UNAUTHORIZED'].includes( | ||
| (j?.status || '').toString().toUpperCase(), | ||
| ), | ||
| ) | ||
| : null; | ||
| if (recentFailure) { | ||
| try { | ||
| if (storageKey && window.storage) | ||
| await window.storage.set(storageKey, null); | ||
| } catch {} | ||
| if (!cancelled) { | ||
| setJobId(null); | ||
| if (setLogsDrawerOpen) setLogsDrawerOpen(true); | ||
| alert( | ||
| `Model start failed: ${recentFailure?.message || recentFailure?.job_data?.error_msg || recentFailure?.status}`, | ||
| ); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| } catch (err) { | ||
| // ignore list errors | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // ignore storage errors and fall back to regular polling | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| const res = await fetch( | ||
| getAPIFullPath('jobs', ['get'], { | ||
| id: currentJobId, | ||
| experimentId: experimentInfo.id, | ||
| }), | ||
| { method: 'GET' }, | ||
| ); | ||
| if (!res.ok) { | ||
| try { | ||
| if (storageKey && window.storage) | ||
| await window.storage.set(storageKey, null); | ||
| } catch {} | ||
| if (!cancelled) setJobId(null); | ||
| return; | ||
| } | ||
| const job = await res.json(); | ||
| const status = (job?.status || '').toString().toUpperCase(); | ||
|
|
||
| // If model is running or completed, clear pending marker and stop buffering | ||
| if ( | ||
| status === 'RUNNING' || | ||
| status === 'COMPLETE' || | ||
| status === 'SUCCESS' | ||
| ) { | ||
| try { | ||
| await window.storage.set(storageKey, null); | ||
| } catch {} | ||
| if (!cancelled) setJobId(null); | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| status === 'FAILED' || | ||
| status === 'STOPPED' || | ||
| status === 'ERROR' || | ||
| status === 'UNAUTHORIZED' | ||
| ) { | ||
| try { | ||
| await window.storage.set(storageKey, null); | ||
| } catch {} | ||
| if (!cancelled) { | ||
| setJobId(null); | ||
| if (setLogsDrawerOpen) setLogsDrawerOpen(true); | ||
| alert( | ||
| `Model start failed: ${job?.message || job?.job_data?.error_msg || status}`, | ||
| ); | ||
| } | ||
| return; | ||
| } | ||
| } catch (e) { | ||
| // ignore transient poll errors | ||
| } | ||
| }; | ||
|
|
||
| // Start polling every 2s and poll immediately | ||
| intervalId = window.setInterval(pollJob, 2000) as unknown as number; | ||
| pollJob(); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| if (intervalId) clearInterval(intervalId); | ||
| }; | ||
| }, [jobId, storageKey, experimentInfo?.id, setLogsDrawerOpen]); | ||
|
|
||
| useEffect(() => { | ||
| if (!storageKey || !window.storage) return; | ||
| (async () => { | ||
| try { | ||
| if (Array.isArray(models) && models.length > 0) { | ||
| await window.storage.set(storageKey, null); | ||
| setJobId(null); | ||
| } | ||
| } catch (e) { | ||
| // ignore storage errors | ||
| } | ||
| })(); | ||
| }, [models, storageKey]); | ||
|
|
||
| const { data, error, isLoading } = useAPI( | ||
| 'experiment', | ||
| ['getScriptsOfTypeWithoutFilter'], | ||
|
|
@@ -296,6 +457,14 @@ export default function RunModelButton({ | |
| return; | ||
| } | ||
|
|
||
| if (storageKey && window.storage) { | ||
| try { | ||
| await window.storage.set(storageKey, -1); | ||
| } catch (e) { | ||
| // ignore storage errors | ||
| } | ||
| } | ||
|
|
||
| setJobId(-1); | ||
|
|
||
| const inferenceEngine = inferenceSettings?.inferenceEngine; | ||
|
|
@@ -310,6 +479,11 @@ export default function RunModelButton({ | |
| experimentInfo?.id, | ||
| ); | ||
| if (response?.status == 'error') { | ||
| if (storageKey && window.storage) { | ||
| try { | ||
| await window.storage.set(storageKey, null); | ||
| } catch (e) {} | ||
| } | ||
| if (setLogsDrawerOpen) { | ||
| setLogsDrawerOpen(true); | ||
| } | ||
|
|
@@ -328,6 +502,13 @@ export default function RunModelButton({ | |
| return; | ||
| } | ||
| const job_id = response?.job_id; | ||
| if (storageKey && window.storage) { | ||
| try { | ||
| await window.storage.set(storageKey, job_id); | ||
| } catch (e) { | ||
| // ignore storage errors | ||
| } | ||
| } | ||
| setJobId(job_id); | ||
| mutate(); | ||
| }} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting this in local storage seems like overkill? i'm guessing there was a problem with just using state?