[ML] Adding file upload tools to package #221034
Open
+1,875
−783
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/ci |
|
/ci |
|
/ci |
jgowdyelastic
added
:ml
release_note:skip
jgowdyelastic
added
backport:version
|
Pinging @elastic/ml-ui (:ml) |
peteharverson
approved these changes
May 29, 2025
darnautov
reviewed
May 30, 2025
Comment on lines
+27
to
+31
| this.infer() | ||
| .then(() => { | ||
| this.inferFinished = true; | ||
| }) | ||
| .catch((e) => { |
There was a problem hiding this comment.
nit: as it is an async function, shall we stick to the await syntax?
| private settings; | ||
| private mappings: MappingTypeMapping | null = null; | ||
| private pipelines: Array<IngestPipeline | undefined> = []; | ||
| public readonly settings$ = new BehaviorSubject<Config<IndicesIndexSettings>>({ |
There was a problem hiding this comment.
Suggested change
| public readonly settings$ = new BehaviorSubject<Config<IndicesIndexSettings>>({ | |
| private readonly settings$ = new BehaviorSubject<Config<IndicesIndexSettings>>({ |
| valid: false, | ||
| count: 0, | ||
| }); | ||
| public readonly existingIndexName$ = new BehaviorSubject<string | null>(null); |
There was a problem hiding this comment.
Suggested change
| public readonly existingIndexName$ = new BehaviorSubject<string | null>(null); | |
| private readonly existingIndexName$ = new BehaviorSubject<string | null>(null); |
| valid: false, | ||
| count: 0, | ||
| }); | ||
| public readonly mappings$ = new BehaviorSubject<Config<MappingTypeMapping>>({ |
There was a problem hiding this comment.
Suggested change
| public readonly mappings$ = new BehaviorSubject<Config<MappingTypeMapping>>({ | |
| private readonly mappings$ = new BehaviorSubject<Config<MappingTypeMapping>>({ |
| count: 0, | ||
| }); | ||
| public readonly existingIndexName$ = new BehaviorSubject<string | null>(null); | ||
|
|
||
| private inferenceId: string | null = null; | ||
| private importer: IImporter | null = null; | ||
| private timeFieldName: string | undefined | null = null; | ||
| private commonFileFormat: string | null = null; | ||
|
|
||
| public readonly uploadStatus$ = new BehaviorSubject<UploadStatus>({ |
There was a problem hiding this comment.
Suggested change
| public readonly uploadStatus$ = new BehaviorSubject<UploadStatus>({ | |
| private readonly uploadStatus$ = new BehaviorSubject<UploadStatus>({ |
| } | ||
|
|
||
| http.get<Index[]>('/api/index_management/indices').then((indx) => { | ||
| setIndices(indx.filter((i) => i.hidden === false && i.isFrozen === false)); |
There was a problem hiding this comment.
- I believe we should only fetch once on mount. We should not re-fetch all indices on
fileUploaderMangerreference change. - We need to check the component is still mounted before setting the state
|
|
||
| useEffect(() => { | ||
| dataViews.getTitles().then((titles) => { | ||
| setExistingDataViewNames(titles); |
There was a problem hiding this comment.
need to check if it's mounted
Comment on lines
+104
to
+105
| uploadStatus.overallImportStatus === STATUS.COMPLETED || | ||
| uploadStatus.overallImportStatus === STATUS.FAILED; |
There was a problem hiding this comment.
I find it confusing that completed and failed considered as in progress. Is it correct?
Comment on lines
+111
to
+115
| fileUploadManager.import(index, dv).then((res) => { | ||
| if (onUploadComplete && res) { | ||
| onUploadComplete(res); | ||
| } | ||
| setImportResults(res); |
There was a problem hiding this comment.
- can by async...await
- is it missing error handling?
- need to check the component is till mounted
| async function updateMappings(index: string, mappings: MappingTypeMapping) { | ||
| const resp = await asCurrentUser.indices.getMapping({ index }); | ||
| const existingMappings = resp[index]?.mappings; | ||
| if (JSON.stringify(existingMappings.properties) !== JSON.stringify(mappings.properties)) { |
There was a problem hiding this comment.
Key order matters with JSON.stringify, so I assume a deep equality check is safer
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moves the core file upload classes to a package called
@kbn/file-uploadMoves a lot of shared and duplicated constants to the package
@kbn/file-upload-commonAdds a new context called
FileUploadContextand related hookuseFileUploadContextto manage all aspects of the file upload process.Updates the file upload lite flyout used by the Search solution to use this new context.