-
Notifications
You must be signed in to change notification settings - Fork 24
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
Batch nml task upload to avoid uploading too many nmls with one request #6216
Conversation
try { | ||
if (this.state.specificationType === SpecificationEnum.Nml) { | ||
// Workaround: Antd replaces file objects in the formValues with a wrapper file | ||
// The original file object is contained in the originFileObj property | ||
// This is most likely not intentional and may change in a future Antd version | ||
// @ts-expect-error ts-migrate(7006) FIXME: Parameter 'wrapperFile' implicitly has an 'any' ty... Remove this comment to see the full error message | ||
formValues.nmlFiles = formValues.nmlFiles.map((wrapperFile) => wrapperFile.originFileObj); | ||
response = await createTaskFromNML(formValues); | ||
const nmlFiles = formValues.nmlFiles.map((wrapperFile) => wrapperFile.originFileObj); | ||
for (let i = 0; i < nmlFiles.length; i += NUM_TASKS_PER_BATCH) { | ||
const batchOfNmls = nmlFiles.slice(i, i + NUM_TASKS_PER_BATCH); | ||
formValues.nmlFiles = batchOfNmls; | ||
// eslint-disable-next-line no-await-in-loop | ||
const response = await createTaskFromNML(formValues); | ||
taskResponses = taskResponses.concat(response.tasks); | ||
warnings = warnings.concat(response.warnings); | ||
} | ||
} else { | ||
if (this.state.specificationType !== SpecificationEnum.BaseAnnotation) { | ||
// Ensure that the base annotation field is null, if the specification mode | ||
// does not include that field. | ||
formValues.baseAnnotation = null; | ||
} | ||
|
||
response = await createTasks([formValues]); | ||
({ tasks: taskResponses, warnings } = await createTasks([formValues])); | ||
} | ||
|
||
handleTaskCreationResponse(response); | ||
handleTaskCreationResponse({ | ||
tasks: taskResponses, | ||
warnings: _.uniq(warnings), | ||
}); |
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.
Note that this includes quite some code duplication compared to the task_create_bulk_view.
I chose to keep the duplication as the code diverges in a few parts like the kind of request that is triggered. I thought that it might be quite difficult to merge these two behaviors neatly together while keeping the code understandable.
=> But feel free to argue with my decision and suggest how this behavior could be unified and deduplicated.
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.
Works like a charm 🎉
I agree that avoiding the bit of code duplication would be rather difficult and is not worth it.
Co-authored-by: Daniel <[email protected]>
This PR batches the task creation via nml to a maximum of 100 per request as this is the limit set by the backend. Previously, trying to upload more than 100 nmls at once resulted in an error. Now the batching prevents this error and triggers one request after another until all nmls are uploaded.
URL of deployed dev instance (used for testing):
Steps to test:
tracing.nml.zip. Extract this nml into an empty folder. In that folder create a
.sh
script and past the following code in that file:for i in {1..150}; do cp tracing.nml "tracing$i.nml"; done
. This code will duplicate the nml file 150 times. Then execute the script.in the top right corner and use the
Create Task` Tab. There fill out the form with the newly created hybrid task type.Task Specification
in the form select theUpload NML File
option.Create Task
button. This should succeed.Issues:
(Please delete unneeded items, merge only when none are left open)