Optical images handling and some other changes #6

Ren22 · 2018-02-14T08:48:24Z

Changes and additions to delete optical image (imageupload.js and resolvers)
Schema additions to retrieve fdr numbers
Other fixes

Rel v0.8

iprotsyuk · 2018-02-14T12:46:36Z

config/test.js

@@ -2,7 +2,7 @@ let config = {};

 config.port = 3011;
 config.ws_port = 5667;
-config.iso_img_port = 4202;
+config.iso_img_port = 4201;


any reasons for this?

I used a different port (4202) for testing on purpose to avoid conflicts with the dev version running locally.

I think I changed it by mistake

iprotsyuk · 2018-02-14T12:57:21Z

resolvers.js

+      let {datasetId} = args;
+      const payload = jwt.decode(args.jwt, config.jwt.secret);
+      try {
+        logger.info(args);


Maybe worth skipping this, or at least printing the requested dataset ID instead of the whole payload

iprotsyuk · 2018-02-14T13:03:03Z

resolvers.js

+        const body = JSON.stringify({
+            id: datasetId
+        });
+        await fetch(url, {


I'd check the return value here as per MDN, since the query may not have been successful even if no exception has been thrown

iprotsyuk · 2018-02-14T13:07:02Z

resolvers.js

-                 logger.error(e);
-               })
+      // zoom = -1 a trick to get all optical images IDs for a single dataset
+      if (zoom == -1) {


Instead of introducing an obscure magic number "-1" here, I'd change the schema.graphql to remove default value for zoom (it's 1 currently), thereby allowing it to be undefined, and add a comment to the schema that undefined is supposed to return image IDs for all zoom levels.

Alternatively, one may simply move this new code into another method, since the return type is actually different in this case: array of strings vs string. At least I don't see any advantage of having these two queries hidden behind the same GraphQL call

@Ren22 is it really needed? You can get all optical image ids for a dataset inside deleteOpticalImage, right?

@intsco hmm, that's true, I just thought that there is a difference in what each methods does. Basically,

deleteOpticalImage
fetches POST query to sm-engine, in which afterwards del_optical_image method is called to delete all entries in DB

opticalImageUrl
just returns strings of image IDs in optical_image folder (in case of zoom=-1) that are further handled by web-app, which sends for every ID DELETE request to be deleted from File system (handled by Express app.delete). I think in any case retrieving all ids is needed here, or maybe there is a better way for that?

@iprotsyuk another method not to mess up it with the current one is a good idea, thanks! I also though of removing value for zoom to undefined, but then I thought that it may cause some unpredictable behavior, that's why I decided to pick -1.

@Ren22, I'd suggest concentrating all deletion actions at one place, which sm-engine (del_optical_image hanlder) seems to be a best place for. Otherwise, one single action of image removal gets too much distributed

iprotsyuk · 2018-02-14T13:13:05Z

schema.graphql

@@ -70,6 +70,7 @@ type Dataset {
  status: JobStatus
  inputPath: String
  uploadDateTime: String
+  fdrCounts: String


I'd propose introducing a small data type (with fields fdrLevel & annotationCount) for the return value here and letting GraphQL do all format handling on its own instead of manually doing stringify and parse

intsco · 2018-02-14T13:20:33Z

imageUpload.js

          await fs.unlink(imgPath);
+          await fs.remove(dirPath);


@Ren22 why to remove the whole subdirectory? It might contain other images.

Is there a way to have more than one image in the same folder? I just found it to be so that for every image there's only one corresponding folder. But this is easy to change

All deletion steps are done in one method async deleteOpticalImage using async/await Added extra type FdrCountsNumber to GraphQL schema

intsco · 2018-02-20T14:16:09Z

@Ren22 please check if optical images get deleted when a datasets is being deleted

iprotsyuk · 2018-02-20T14:24:35Z

resolvers.js

+
+    fdrCounts(ds) {
+      // Get 10% FDR string -> counts[1], 5% -> counts[0]
+      let fdrAt10Val = ds._source.annotation_counts[0].counts[1];


The implementation is too specific here. One of ways to make it more general and more useful in the future is adding the needed FDR level as a parameter

Given the name of the method I'd expect it to return a map of the form fdr_level -> count_n
I thought we want to display this information in the web app on the datasets page

I agree with the expectation here, but since we don't really need such a map anywhere, I'd transform it to a bit more specific function

Yes, I think the initial idea was to get all FDR levels with number of counts for each of them. However, after discussion with Theo , only 10% FDR counts were chosen to be shown. Adding FDR level as a param would make this code much more flexible. Another option is to return all FDR levels with counts and then decide on the web-app side, which level to show. I think the latter is the better option, becasue then we can allow user to chose for instance for the level himself w/o extra requests to GQL. What do you think @iprotsyuk @intsco ?

I guess if there're no other FDR-related feature requests for the web-app which could reuse these GQL endpoints, I wouldn't go for the general solution, as it brings more unnecesary data transfer over the network. We can always add the general method once it's needed. So I'd stick to a parametrical FDR count request for the time being.

Got it, thank you @intsco @iprotsyuk
So, I will send all 4 pairs of FDR with counts to web-app with only 10% displayed at the moment.

@Ren22 you could get the best of all worlds and accept an array of fdr values to return as an input...

thanks @andy-d-palmer! will do that

@Ren22, bear in mind that this approach is a bit complicated with argument handling. What if requested FDR level is not one of the pre-computed ones by SM-engine, including cases when it's outside of [0, 100]% interval. Also there's unnecessary degree of freedom in a sense that values in input array can be repeated. Overall, it seems that returning all available FDR counts is more preferable option here.

@iprotsyuk great points about the input handling. I added a feature request for making the backend less coupled to the web app (i.e. storing float fdr values rather than discrete levels for display) so it makes sense to have more general api.

iprotsyuk · 2018-02-20T14:27:23Z

resolvers.js

+        await checkPermissions(datasetId, payload);
+        let opticalImageIds = await pg.select('id').from('optical_image').where('ds_id', '=', datasetId);
+        let rawOpticalImage = await pg.select('optical_image').from('dataset').where('id','=',datasetId);
+        Array.prototype.push.apply(opticalImageIds, rawOpticalImage);


I'd go for concat() here to make it more readable

Changed it, thanks @iprotsyuk
Also seems that concat is a bit faster than Array.prototype.push.apply for Safari
just fyi https://jsperf.com/array-prototype-push-apply-vs-concat/17

intsco · 2018-02-20T14:21:00Z

resolvers.js

+        opticalImageIds.forEach((Image) => {
+          if (Image.id){ //to delete zoomed Image
+            let url = basePath + `${config.img_upload.categories.optical_image.path}delete/${Image.id}`;
+            fetch(url, {


@Ren22 Try to await for the result of "fetch" to ensure that exceptions inside a promise are handled well.
And call "fetch" on an incorrect url to see if an exception is raised here and handled below.

intsco · 2018-02-20T14:36:14Z

schema.graphql

+  level: Int
+  counts: Int
+}
+


The type might also need to be updated. See the comment above

intsco · 2018-02-20T14:36:36Z

schema.graphql

@@ -70,6 +75,7 @@ type Dataset {
  status: JobStatus
  inputPath: String
  uploadDateTime: String
+  fdrCounts: FdrCountsNumber


The field might also need to be updated. See the comment above

iprotsyuk · 2018-02-20T14:36:57Z

resolvers.js

+        let opticalImageIds = await pg.select('id').from('optical_image').where('ds_id', '=', datasetId);
+        let rawOpticalImage = await pg.select('optical_image').from('dataset').where('id','=',datasetId);
+        Array.prototype.push.apply(opticalImageIds, rawOpticalImage);
+        opticalImageIds.forEach((Image) => {


I'd change the logic here a little bit to reduce the code duplication.

how it works now:

fetch IDs of optical images

fetch ID of raw image

merge the arrays of IDs into 1 array

iterate over the array, and depending on the type of image ID send DELETE request to a specific URL

how I'd put it:

fetch IDs of optical images

generate an array of URLs to send DELETE request to

fetch ID of raw image

generate 1 more URL to send DELETE request to

send DELETE requests to each of generated URLs

Thanks @iprotsyuk , I will send changes with a new commit (coming)

iprotsyuk · 2018-02-20T14:48:49Z

schema.graphql

@@ -70,6 +75,7 @@ type Dataset {
  status: JobStatus
  inputPath: String
  uploadDateTime: String
+  fdrCounts: FdrCountsNumber


I guess you should decide whether counts for all FDR levels should be returned by this field (then the use of this custom type FdrCountsNumber is perfectly fine), or a count for specific FDR level is returned (then the level property doesn't make much sense).

Based on the current usage of this field (we don't need counts for all FDR levels), I'd suggest putting it as a function here with fdrLevel parameter, s.t. it'd return just one number.

changes after 2nd code review

Ren22 · 2018-02-22T07:32:44Z

Changes after 2nd code review:

Rewritten deleteOpticalImage with only one fetch request per each URL
fdrCounts now takes a min and max value of fdrLevel and should return the list of all fdrCounts between them.

intsco · 2018-02-22T09:02:42Z

resolvers.js

+      let allLvlCounts = ds._source.annotation_counts[0].counts;
+      let fdrLevels = [];
+      let fdrCounts = [];
+      allLvlCounts.forEach(Level => {


variables and argument names should start with a small letter

intsco · 2018-02-22T09:05:09Z

resolvers.js

+        await checkPermissions(datasetId, payload);
+        let opticalImageIds, rawOpticalImage = await pg.select('id').from('optical_image').where('ds_id', '=', datasetId)
+        let rawOpticalImage = await pg.select('optical_image').from('dataset').where('id', '=', datasetId);
+        await opticalImageIds.forEach(Image => {


arrow function arguments naming
see more here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

intsco · 2018-02-22T09:16:21Z

schema.graphql

@@ -44,6 +44,11 @@ enum JobStatus {
  FAILED
 }

+type fdrCountsNumber {


GraphQL types must start with a capital letter
see examples http://graphql.org/learn/schema/#object-types-and-fields

@intsco thanks for pointing on that problem and links. I will correct it

… deleted, other small fixes

Processing requested Fdrlevels, deletion of opt Image

iprotsyuk · 2018-02-28T13:02:22Z

schema.graphql

@@ -44,6 +44,11 @@ enum JobStatus {
  FAILED
 }

+type FdrCountsNumber {


I'd rename it to FdrCounts as it's shorter and conveys the same meaning

iprotsyuk · 2018-02-28T13:02:37Z

schema.graphql

@@ -70,6 +75,7 @@ type Dataset {
  status: JobStatus
  inputPath: String
  uploadDateTime: String
+  fdrCounts(inpFdrLvls: [Float]): FdrCountsNumber


I'd use ! as a non-null specification everywhere in this declaration and definition of FdrCountsNumber

Thanks @iprotsyuk, I will add it

iprotsyuk · 2018-02-28T13:08:46Z

schema.graphql

@@ -235,7 +241,7 @@ type Query {
  allDatasets(orderBy: DatasetOrderBy = ORDER_BY_DATE,
              sortingOrder: SortingOrder = DESCENDING,
              filter: DatasetFilter = {}, simpleQuery: String,
-              offset: Int = 0, limit: Int = 10): [Dataset!]!
+              offset: Int = 0, limit: Int = 10, inpFdrLvls: [Float]): [Dataset!]!


Somehow, the change in the function signature didn't affect its implementation. Forgot to remove?

Yes, that's indeed a redundant argument that I passed while experimenting with an input, will delete it

iprotsyuk · 2018-02-28T13:20:38Z

resolvers.js

+          allUrls.push(basePath + `${config.img_upload.categories.raw_optical_image.path}delete/${Image.optical_image}`)
+        });
+        await allUrls.forEach(url => {
+          fetch(url, {


I think I suggested in one of prior reviews to check the return value of fetch as it can be a sign of unsuccessful query as well.

I thought that calling catch on a promise returned from fetch is enough for that

I think what I'm missing here is checking for a response.ok as per MDN fetch promise is rejected when there's network problem.

An accurate check for a successful fetch() would include checking that the promise resolved, then checking that the Response.ok property has a value of true.

fetch(url).then((response) => { if (response.ok) { //do smth } else { throw new Error('Something went wrong'); } }) .catch((error) => { console.log(error) });

Would it be correct? @intsco @iprotsyuk

yep, I think so

iprotsyuk · 2018-02-28T13:22:36Z

resolvers.js

+        });
+        return {
+          'levels': outfdrLvls,
+          'counts': outFdrCounts


outfdrLvls & outFdrCounts can be undefined if the dataset status is not FINISHED

iprotsyuk · 2018-02-28T13:27:22Z

resolvers.js

+        let inpAllLvlCounts = ds._source.annotation_counts[0].counts;
+        let outfdrLvls = [], outFdrCounts = [];
+        inpFdrLvls.forEach(lvl => {
+          for (let lvlCount of inpAllLvlCounts) {


I'd use inpAllLvlCounts.find() instead of iteration through the whole array. It'd do the same thing, but read better

fixes to fdrCount, fetch function handler

iprotsyuk · 2018-03-02T13:09:46Z

resolvers.js

@@ -77,6 +77,15 @@ function baseDatasetQuery() {
  });
 }

+function checkFetchRes(resp) {
+  if (resp.status >= 200 && resp.status < 300) {


I'd check resp.ok flag here, as it has the same meaning as per MDN

@iprotsyuk that's the interesting case, what I found with fetch is that resp will be ok even if 404 status is raised. e.g. when one sends a full url , so this was a way to catch it that I came up with
More:
JakeChampion/fetch#155

actually I checked it once again and seems that resp.ok works fine too. Don't know what was the thing before

iprotsyuk · 2018-03-02T13:15:44Z

resolvers.js

+      if(ds._source.annotation_counts && ds._source.ds_status === 'FINISHED') {
+        let inpAllLvlCounts = ds._source.annotation_counts[0].counts;
+        inpFdrLvls.forEach(lvl => {
+          inpAllLvlCounts.find(lvlCount => {


It's a bit confusing usage of find(). The callback function should just check that lvlCount.level === lvl. Then, find() would return the corresponding count, and you extract necessary data from it. Keep in mind that find() may return undefined.

iprotsyuk · 2018-03-02T13:22:15Z

resolvers.js

+        await allUrls.push(basePath +
+            `${config.img_upload.categories.raw_optical_image.path}delete/${rawOpticalImage[0].optical_image}`);
+        for (let image of opticalImageIds) {
+          await allUrls.push(basePath + `${config.img_upload.categories.optical_image.path}delete/${image.id}`)}


It seems push() is synchronous, so I'd remove await from this line and the one above. Also please put the closing brace to a new line :)

iprotsyuk · 2018-03-02T13:23:25Z

resolvers.js

+        for (let url of allUrls) {
+          let res = await fetch(url, {
+            method: 'delete'});
+          await checkFetchRes(res);


checkFetchRes is also synchronous, so await doesn't do anything

true, thanks for pointing on that @iprotsyuk

iprotsyuk · 2018-03-02T13:28:02Z

A general comment: it seems that optical image removal doesn't take place upon dataset deletion. If this is the case indeed, It should be addressed in another PR to sm-engine.

Ren22 · 2018-03-02T13:40:58Z

@iprotsyuk it is properly deleted by daleteDatasetQuery from sm-web app without messing things up in resolver on GQL, I will commit it soon :) (but I think I already added it in the previous commit to sm-web-app)

fixes

iprotsyuk · 2018-03-02T19:18:45Z

👍

Moved all fs and db mutations to sm-eng

got rid off unnecessary variables

…ts resolver

intsco · 2018-03-19T10:26:37Z

Merged into master as a single commit (cb97991). The PR has too many merge commits

root and others added 6 commits February 6, 2018 11:12

Image and directory deletion added

33f62f7

Added mutation and resolver to delete opt_image

25dadb8

eseacrh resolvers and schema changes

1371678

Fixes after local review

95361ae

Fixes after local review

2a09699

Merge pull request #1 from Ren22/rel-v0.8

2cd7fd5

Rel v0.8

iprotsyuk reviewed Feb 14, 2018

View reviewed changes

intsco reviewed Feb 14, 2018

View reviewed changes

Ren22 added 3 commits February 19, 2018 19:37

Changes after code review

adf74db

Deletion -1 related extra code

975c895

Merge pull request #2 from Ren22/rel-v0.8

b70af6f

All deletion steps are done in one method async deleteOpticalImage using async/await Added extra type FdrCountsNumber to GraphQL schema

iprotsyuk reviewed Feb 20, 2018

View reviewed changes

intsco reviewed Feb 20, 2018

View reviewed changes

schema.graphql Outdated

level: Int

counts: Int

}

Copy link

Member

intsco Feb 20, 2018

Choose a reason for hiding this comment

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

The type might also need to be updated. See the comment above

intsco reviewed Feb 20, 2018

View reviewed changes

iprotsyuk reviewed Feb 20, 2018

View reviewed changes

Ren22 added 2 commits February 22, 2018 08:03

changes after 2nd code review

ee25719

Merge pull request #3 from Ren22/rel-v0.8

1e78e3c

changes after 2nd code review

intsco reviewed Feb 22, 2018

View reviewed changes

Processing requested Fdrlevels, deletion of opt Image when dataset is…

21dc8be

… deleted, other small fixes

Merge pull request #4 from Ren22/rel-v0.8

a6631cc

Processing requested Fdrlevels, deletion of opt Image

iprotsyuk reviewed Feb 28, 2018

View reviewed changes

Ren22 added 2 commits March 2, 2018 13:45

fixes to fdrCount, fetch function handler

df0df64

Merge pull request #5 from Ren22/rel-v0.8

9d30af1

fixes to fdrCount, fetch function handler

iprotsyuk reviewed Mar 2, 2018

View reviewed changes

Ren22 added 2 commits March 2, 2018 19:31

fixes

28b3282

Merge pull request #6 from Ren22/rel-v0.8

6150248

fixes

Ren22 added 4 commits March 2, 2018 20:25

moved all fs and db mutations to sm-eng

899e9b2

Merge pull request #7 from Ren22/rel-v0.8

edbf965

Moved all fs and db mutations to sm-eng

got rid off unnecessary variables

d90f34e

Merge pull request #8 from Ren22/rel-v0.8

9e05ca0

got rid off unnecessary variables

intsco added a commit that referenced this pull request Mar 19, 2018

Merge branch 'Ren22-master' (PR #6): optical images deletion, fdrCoun…

cb97991

…ts resolver

intsco closed this Mar 19, 2018

Optical images handling and some other changes #6

Optical images handling and some other changes #6

Conversation

Ren22 commented Feb 14, 2018

iprotsyuk Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

intsco Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iprotsyuk Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intsco commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iprotsyuk Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intsco Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ren22 commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iprotsyuk Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iprotsyuk Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Ren22 Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iprotsyuk commented Mar 2, 2018

Ren22 commented Mar 2, 2018

iprotsyuk commented Mar 2, 2018

intsco commented Mar 19, 2018 • edited Loading

iprotsyuk Feb 14, 2018 •

edited

Loading

intsco Feb 14, 2018 •

edited

Loading

iprotsyuk Feb 14, 2018 •

edited

Loading

iprotsyuk Feb 20, 2018 •

edited

Loading

intsco Feb 20, 2018 •

edited

Loading

iprotsyuk Feb 28, 2018 •

edited

Loading

iprotsyuk Mar 2, 2018 •

edited

Loading

Ren22 Mar 2, 2018 •

edited

Loading

intsco commented Mar 19, 2018 •

edited

Loading