Labels.txt Upload by marySalvi · Pull Request #1072 · Kitware/dive

marySalvi · 2021-11-22T19:39:44Z

Fixes #599
Currently on web platform only:
Adds ability for user to upload a labels.txt file via the training menu
If the file is present, writes to the temp directory and sends the file to viame with the run_training task

subdavis

This looks good. Two main comments:

When you add new optional parameters to a method, I think it's best to add them at the end rather than in the middle. It breaks the previous API unnecessarily, and if you add them at the end, you don't risk creating a bug somewhere that you forgot to update the function call.
I think the text argument needs to be added into the request body (comment below)

subdavis · 2021-11-22T19:41:52Z

 import { CustomStyle } from 'vue-media-annotator/use/useStyling';

-type DatasetType = 'image-sequence' | 'video' | 'multi';
+type DatasetType = 'image-sequence' | 'video' | 'multi' | 'txt';


'txt' shouldn't be a dataset type. This is like an image sequence, a video, or a multi-camera dataset.

should be set now

subdavis · 2021-11-22T19:43:49Z

-    folderIds: string[], pipelineName: string, config: string, annotatedFramesOnly: boolean
+    folderIds: string[],
+    pipelineName: string,
+    labelText: string | null,


Optional params should probably move to the end of the method and become labelText?: string

If you make it a new optional param at the end, the API spec will be backward compatible.

should be set now

subdavis · 2021-11-22T19:44:45Z

  [ImageSequenceType]: 'image sequence',
  [VideoType]: 'video',
  [MultiType]: 'multi',
+  [TxtType]: 'txt',


I don't think you want to modify MediaTypes at all for this feature. No new dataset types are being added.

should be set now

subdavis · 2021-11-22T19:45:25Z

 const ImageSequenceType = 'image-sequence';
 const VideoType = 'video';
 const MultiType = 'multi';
+const TxtType = 'txt';


Probably remove?

should be set now

subdavis · 2021-11-22T19:49:00Z

    source_folder_list: List[GirderModel],
    groundtruth_list: List[GirderModel],
    pipeline_name: str,
+    label_text:str,


I think this should be an optional param and move to the end:

label_text: Optional[str] = None

should be set now

subdavis · 2021-11-22T19:58:06Z

      if (dataset.value === null) return null;
      const { type } = dataset.value;
      if (type === MultiType) throw new Error('Cannot export multicamera dataset');
+      if (type === TxtType) throw new Error('Cannot export txt dataset');


I think this can be removed.

should be set now

subdavis · 2021-11-22T19:59:04Z

    newjob.job[constants.JOBCONST_PRIVATE_QUEUE] = job_is_private
    newjob.job[constants.JOBCONST_TRAINING_INPUT_IDS] = folderIds
    newjob.job[constants.JOBCONST_RESULTS_FOLDER_ID] = str(results_folder['_id'])
+    newjob.job[constants.JOBCONST_LABEL_TEXT] = labelText


Just FYI, these meta values aren't really used for anything. They're here because we thought it would be better to have this information than not. No need to change, though.

subdavis · 2021-11-22T20:04:01Z

+            <v-file-input
+              v-if="labelText"
+              v-model="labelFile"
+              clearable
+            />
+            <import-button
+              name="Upload labels.txt File"
+              icon="mdi-folder-open"
+              open-type="txt"
+              class="grow"
+              @open="openTxt"
+            />


Two notes:

Is the import button necessary, or could we just use the v-file-input here? In other places in the app where file inputs are expected (Such as annotation file during import) only the v-file-input is used.

I don't think it's clear to the user what this is or that it's optional. Perhaps we need a short description and a link to documentation. I don't think matt has written any documentation about this file, so we should probably add it somewhere in our docs.

I have changed the UI to use v-file-input instead of the import button. This also removes the need to use the OpenFromDisk method.
I added the optional verbiage but I still don't feel I understand the contents of labels.txt file enough to provide meaningful documentation on it.

should be set now

subdavis · 2021-11-22T20:05:24Z

      trainingOutputName.value = null;
    }

+    const openTxt = async () => {


Please use async function function definition for consistency with line 60.

should be set now

subdavis · 2021-11-22T20:09:24Z

+        };
+        reader.readAsText(data.fileList[0]);
+        // eslint-disable-next-line prefer-destructuring
+        labelFile.value = data.fileList[0];


Suggested change

labelFile.value = data.fileList[0];

[labelFile.value] = data.fileList;

This will resolve the eslint warning so you can remove the disable comment.

should be set now

BryonLewis

I generally agree that the datatype for datasets shouldn't be modified and that file-input should work fully.

This is just a note I believe about labels.txt from testing. The labels used have to be present in all training datasets I think? I was getting an error if they weren't. This is probably a question for Matt.

…c function

marySalvi · 2021-12-10T13:59:55Z

For the record I was calling the fact that I missed that linting issue silly, not that the rule existed. I don't have strong opinions about the rule but love the idea of black fixing it for us.

BryonLewis

Just asking about removing the 'txt' from openFromDisk if it is no longer being used.

Other than that it seems to load and run fine. I was able to get two pipelines trained which presented different results based on having a labels.txt to restrict the training vs letting it train with no labels.txt.

BryonLewis · 2021-12-10T13:34:22Z

  saveAttributes(datasetId: string, args: SaveAttributeArgs): Promise<unknown>;
  // Non-Endpoint shared functions
-  openFromDisk(datasetType: DatasetType | 'calibration' | 'annotation' | 'text', directory?: boolean):
+  openFromDisk(datasetType: DatasetType | 'calibration' | 'annotation' | 'txt' | 'text', directory?: boolean):


Is this necessary now that we are no longer using openFromDisk?

should be set now

BryonLewis · 2021-12-10T13:35:09Z

+  } else if (datasetType === 'txt') {
+    input.accept = inputTxtTypes;


Now that txt is no longer being used is this necessary to keep in this PR?

You are absolutely right, that can be removed now

should be set now

BryonLewis

Took another look at stuff, only changes from last review should be that removal. As a precaution I pulled/built and ran integration testing as well as looked at some basic training results. Looks good to me.

subdavis · 2021-12-17T21:41:26Z

I'd like to combine #1092 into this, and then this is good to go!

* WIP * Update Pipeline-Documentation.md

subdavis

marySalvi requested review from BryonLewis and subdavis November 22, 2021 19:39

subdavis suggested changes Nov 22, 2021

View reviewed changes

BryonLewis requested changes Nov 23, 2021

View reviewed changes

Comment thread client/platform/web-girder/views/RunTrainingMenu.vue Outdated

Comment thread client/platform/web-girder/views/RunTrainingMenu.vue

marySalvi force-pushed the labelsUpload branch from 381c756 to 8839245 Compare December 9, 2021 04:35

marySalvi added 14 commits December 8, 2021 23:35

prelim work

24e26ff

add import of labels.txt to training menu

38c7df2

add labels.text as type and parameter

34d3791

start work on adding labels.txt to viame run training

7108dd3

add labelText to desktop calls

e1a1374

update task for label_text

8645f0d

make labels command conditional on labels file existence

9da39df

lint and tox

7938aea

make labelText optional parameter

1b64411

remove unnecessary TxtType and labelText values

60d6951

make return for runTrainingOnFolder conditional and standardized asyn…

c5a28b5

…c function

correct params order for labelText to be last and optional

38682cb

send labelText as part of body of request

97c081e

remove import button and openFromDisk method

58dde9e

marySalvi force-pushed the labelsUpload branch from 8839245 to 58dde9e Compare December 9, 2021 04:35

marySalvi requested review from BryonLewis and subdavis December 9, 2021 04:43

remove incorrect test type

559ef32

marySalvi commented Dec 9, 2021

View reviewed changes

Comment thread server/dive_server/views_rpc.py Outdated

marySalvi added 2 commits December 9, 2021 16:25

fix lint error and create pydantic class for training args

7754c58

fix silly lint error

60c2fd7

BryonLewis requested changes Dec 10, 2021

View reviewed changes

remove unnecessary txt typing

2c6f216

marySalvi requested a review from BryonLewis December 14, 2021 18:01

BryonLewis previously approved these changes Dec 16, 2021

View reviewed changes

Add Documentation for Labels.txt (#1092)

b31ec8b

* WIP * Update Pipeline-Documentation.md

subdavis dismissed BryonLewis’s stale review via b31ec8b December 20, 2021 15:31

subdavis approved these changes Dec 20, 2021

View reviewed changes

marySalvi merged commit f63ad12 into main Dec 20, 2021

marySalvi deleted the labelsUpload branch December 20, 2021 19:46

BryonLewis mentioned this pull request Sep 8, 2025

Desktop Labels.txt support #1511

Closed

	labelFile.value = data.fileList[0];
	[labelFile.value] = data.fileList;

		} else if (datasetType === 'txt') {
		input.accept = inputTxtTypes;

Conversation

marySalvi commented Nov 22, 2021

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marySalvi commented Dec 10, 2021

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

subdavis commented Dec 17, 2021

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone