Skip to content
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

[ML] Initialize saved objects on trained model page load #201426

Merged
merged 5 commits into from
Nov 27, 2024
Merged

[ML] Initialize saved objects on trained model page load #201426

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
029bcbc
[ML] Initialize saved objects on trained model page load
jgowdyelastic Nov 22, 2024
dd51cd5
Merge branch 'main' into initialize-saved-objects-on-trained-model-pa…
jgowdyelastic Nov 25, 2024
d3161c8
fixing sync for non-admin users
jgowdyelastic Nov 25, 2024
49b6fff
Merge branch 'main' into initialize-saved-objects-on-trained-model-pa…
jgowdyelastic Nov 27, 2024
04c567a
Merge branch 'main' into initialize-saved-objects-on-trained-model-pa…
jgowdyelastic Nov 27, 2024
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
18 changes: 16 additions & 2 deletions x-pack/plugins/ml/public/application/routing/routes/trained_models/models_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import type { FC } from 'react';
import { type FC, useCallback } from 'react';
import React from 'react';
import { FormattedMessage } from '@kbn/i18n-react';
import { i18n } from '@kbn/i18n';
Expand All @@ -19,6 +19,7 @@ import { useRouteResolver } from '../../use_resolver';
import { basicResolvers } from '../../resolvers';
import { getBreadcrumbWithUrlForApp } from '../../breadcrumbs';
import { MlPageHeader } from '../../../components/page_header';
import { useSavedObjectsApiService } from '../../../services/ml_api_service/saved_objects';

const ModelsList = dynamic(async () => ({
default: (await import('../../../model_management/models_list')).ModelsList,
Expand Down Expand Up @@ -48,7 +49,20 @@ export const modelsListRouteFactory = (
});

const PageWrapper: FC = () => {
const { context } = useRouteResolver('full', ['canGetTrainedModels'], basicResolvers());
const { initSavedObjects } = useSavedObjectsApiService();

const initSavedObjectsWrapper = useCallback(async () => {
try {
await initSavedObjects();
} catch (error) {
// ignore error as user may not have permission to sync
}
}, [initSavedObjects]);

const { context } = useRouteResolver('full', ['canGetTrainedModels'], {
...basicResolvers(),
initSavedObjectsWrapper,
});
Comment on lines +52 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: useRouteResolver takes care of preserving refs for provided callbacks:

Suggested change
const { initSavedObjects } = useSavedObjectsApiService();
const initSavedObjectsWrapper = useCallback(async () => {
try {
await initSavedObjects();
} catch (error) {
// ignore error as user may not have permission to sync
}
}, [initSavedObjects]);
const { context } = useRouteResolver('full', ['canGetTrainedModels'], {
...basicResolvers(),
initSavedObjectsWrapper,
});
const { initSavedObjects } = useSavedObjectsApiService();
const { context } = useRouteResolver('full', ['canGetTrainedModels'], {
...basicResolvers(),
initSavedObjects: () => initSavedObjects().catch(() => {})
});

Copy link
Member Author

Choose a reason for hiding this comment

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

This change is neater. I'm currently working on a related change which will do something similar on all ML pages. So for the sake of not having to retest this PR I'm going to merge as is and apply this refactor to the follow up PR


return (
<PageLoader context={context}>
Expand Down