Skip to content

[ML] Improving model snapshot revert UI experience #88588

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 21, 2021
Merged

[ML] Improving model snapshot revert UI experience #88588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e897daa
[ML] Improving model snapshot revert UI experience
jgowdyelastic Jan 18, 2021
bd0ae2d
removing button disabling
jgowdyelastic Jan 18, 2021
6f1867d
updating component is mounted check
jgowdyelastic Jan 19, 2021
2c1e579
Merge branch 'master' into improve-model-snapshot-revert-UI-experience
kibanamachine Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions x-pack/plugins/ml/public/application/components/model_snapshots/model_snapshots_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,24 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => {
const [closeJobModalVisible, setCloseJobModalVisible] = useState<ModelSnapshot | null>(null);
const [combinedJobState, setCombinedJobState] = useState<COMBINED_JOB_STATE | null>(null);

let unmounted = false;
Copy link
Contributor

@walterra walterra Jan 19, 2021

Choose a reason for hiding this comment

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

To avoid the let and resetting the variable on every render we use this alternative pattern in some places: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ml/public/application/data_frame_analytics/pages/analytics_exploration/components/outlier_exploration/use_outlier_data.ts#L81

We discussed making a reusable custom hook in the past but didn't get around to do it. Here's the PR that originally introduced it: #81123

Copy link
Member Author

@jgowdyelastic jgowdyelastic Jan 19, 2021

Choose a reason for hiding this comment

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

I had a look at other similar examples in our code, but could not find one where the guard variable is used in a separate function and so needed to in the outer scope.

I might missing something, but using a const wrapper around the variable won't make any difference? Both will be redefined on render.

let unmounted = false;

vs

const options = { unmounted: false };

Copy link
Contributor

@walterra walterra Jan 19, 2021

Choose a reason for hiding this comment

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

You're right this is different because of the additional outside callback. In the other examples the difference was that everything could be kept within the useEffect.

As an alternative you could use a refresh value to trigger the useEffect instead of directly calling loadModelSnapshots().

Something like:

const [isRefreshed, setIsRefreshed] = useState(true);

useEffect(() => {
    const options = { didCancel: false };
    loadModelSnapshots(options);
    return () => {
      options.didCancel = true;
    };
  }, [isRefreshed]);

async function loadModelSnapshots(options) {
    if (options.didCancel === true) {
      // table refresh can be triggered a while after a snapshot revert has been triggered.
      // ensure the table is still visible before attempted to refresh it.
      return;
    }
    const { model_snapshots: ms } = await ml.getModelSnapshots(job.job_id);
    setSnapshots(ms);
    setSnapshotsLoaded(true);
}

const refresh = useCallback(
    () => {
        setIsRefreshed(state => !state);
        // wait half a second before refreshing the jobs list
        setTimeout(refreshJobList, 500);
    }
    [setIsRefreshed],
);

Copy link
Member Author

@jgowdyelastic jgowdyelastic Jan 19, 2021

Choose a reason for hiding this comment

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

I've updated this check so it now uses useRef to keep track of the flag for whether the component is mounted.
in 6f1867d

useEffect(() => {
loadModelSnapshots();
return () => {
unmounted = true;
};
}, []);

const loadModelSnapshots = useCallback(async () => {
async function loadModelSnapshots() {
if (unmounted === true) {
// table refresh can be triggered a while after a snapshot revert has been triggered.
// ensure the table is still visible before attempted to refresh it.
return;
}
const { model_snapshots: ms } = await ml.getModelSnapshots(job.job_id);
setSnapshots(ms);
setSnapshotsLoaded(true);
}, [job]);
}

const checkJobIsClosed = useCallback(
async (snapshot: ModelSnapshot) => {
Expand Down Expand Up @@ -107,13 +116,14 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => {
}
}, []);

const closeRevertFlyout = useCallback((reload: boolean) => {
const closeRevertFlyout = useCallback(() => {
setRevertSnapshot(null);
if (reload) {
loadModelSnapshots();
// wait half a second before refreshing the jobs list
setTimeout(refreshJobList, 500);
}
}, []);

const refresh = useCallback(() => {
loadModelSnapshots();
// wait half a second before refreshing the jobs list
setTimeout(refreshJobList, 500);
}, []);

const columns: Array<EuiBasicTableColumn<any>> = [
Expand Down Expand Up @@ -231,6 +241,7 @@ export const ModelSnapshotTable: FC<Props> = ({ job, refreshJobList }) => {
snapshots={snapshots}
job={job}
closeFlyout={closeRevertFlyout}
refresh={refresh}
/>
)}

Expand Down
48 changes: 28 additions & 20 deletions .../components/model_snapshots/revert_model_snapshot_flyout/revert_model_snapshot_flyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,17 @@ interface Props {
snapshot: ModelSnapshot;
snapshots: ModelSnapshot[];
job: CombinedJobWithStats;
closeFlyout(reload: boolean): void;
closeFlyout(): void;
refresh(): void;
}

export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job, closeFlyout }) => {
export const RevertModelSnapshotFlyout: FC<Props> = ({
snapshot,
snapshots,
job,
closeFlyout,
refresh,
}) => {
const { toasts } = useNotifications();
const { loadAnomalyDataForJob, loadEventRateForJob } = useMemo(
() => chartLoaderProvider(mlResultsService),
Expand Down Expand Up @@ -110,13 +117,6 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
setChartReady(true);
}, [job]);

function closeWithReload() {
closeFlyout(true);
}
function closeWithoutReload() {
closeFlyout(false);
}

function showRevertModal() {
setRevertModalVisible(true);
}
Expand All @@ -138,15 +138,18 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
}))
: undefined;

await ml.jobs.revertModelSnapshot(
job.job_id,
currentSnapshot.snapshot_id,
replay,
end,
events
);
ml.jobs
.revertModelSnapshot(job.job_id, currentSnapshot.snapshot_id, replay, end, events)
.then(() => {
toasts.addSuccess(
i18n.translate('xpack.ml.revertModelSnapshotFlyout.revertSuccessTitle', {
defaultMessage: 'Model snapshot revert successful',
})
);
refresh();
});
hideRevertModal();
closeWithReload();
closeFlyout();
} catch (error) {
setApplying(false);
toasts.addError(new Error(error.body.message), {
Expand All @@ -166,7 +169,7 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,

return (
<>
<EuiFlyout onClose={closeWithoutReload} hideCloseButton size="m">
<EuiFlyout onClose={closeFlyout} hideCloseButton size="m">
<EuiFlyoutHeader hasBorder>
<EuiTitle size="s">
<h5>
Expand Down Expand Up @@ -347,7 +350,7 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
<EuiFlyoutFooter>
<EuiFlexGroup justifyContent="spaceBetween">
<EuiFlexItem grow={false}>
<EuiButtonEmpty iconType="cross" onClick={closeWithoutReload} flush="left">
<EuiButtonEmpty iconType="cross" onClick={closeFlyout} flush="left">
<FormattedMessage
id="xpack.ml.newJob.wizard.revertModelSnapshotFlyout.closeButton"
defaultMessage="Close"
Expand Down Expand Up @@ -395,7 +398,12 @@ export const RevertModelSnapshotFlyout: FC<Props> = ({ snapshot, snapshots, job,
confirmButtonDisabled={applying}
buttonColor="danger"
defaultFocusedButton="confirm"
/>
>
<FormattedMessage
id="xpack.ml.newJob.wizard.revertModelSnapshotFlyout.modalBody"
defaultMessage="The snapshot revert will be carried out in the background and may take some time."
/>
</EuiConfirmModal>
</EuiOverlayMask>
)}
</>
Expand Down