Error reading point annotation

[wbwakeman]

Environment

• OS:

Windows 10

Description When attempting to load a JSON that was saved with the new point annotation feature (PR #169 for issue #161 ), the annotation does not load. There is an error in the terminal:

vtkMRMLMarkupsJsonStorageNode::vtkInternal::ReadMeasurements: Markups measurement reading failed: invalid measurements item (expected it to be an array).

Expected behavior The previously saved annotation should load.

How to Reproduce

1. Start the application from command line for any particular specimen. Here is my example:

   "C:\Users\waynew\AppData\Local\Allen Institute\CellLocator 0.1.0-2021-05-21\CellLocator.exe" --reference-view Coronal --view-angle 107 --lims-specimen-id 1109460686 --lims-base-url http://lims2

2. Make a point annotation and save to LIMS
3. Close the program
4. Rerun the command to start the application for the same specimen
5. The result is that the annotation does not load and the error is displayed in the terminal
6. Logs from a successful load of a polygon annotation and an unsuccessful load for a point annotation are attached.

CellLocator_29814_20210625_091118_272.log CellLocator_29814_20210625_090801_350.log

[allemangD]

@wbwakeman There are some comments in the linked PR. I still need to update the conversion script with that change.

In the meantime, it should be possible to work around this by deleting the measurements: null field entirely, although I haven't tested this with the current Cell Locator release. I'll update this comment once I verify that.

[wbwakeman]

Looks like this is the error that I get if I just remove the "measurements":null, line from the file and try to load it:

Traceback (most recent call last):
  File "C:/Users/waynew/AppData/Local/Allen Institute/CellLocator 0.1.0-2021-05-21/bin/../lib/CellLocator-4.13/qt-scripted-modules/Home.py", line 752, in onLoadAnnotationButtonClicked
    annotations = AnnotationManager.fromFile(fileName)
  File "C:/Users/waynew/AppData/Local/Allen Institute/CellLocator 0.1.0-2021-05-21/bin/../lib/CellLocator-4.13/qt-scripted-modules/Home.py", line 452, in fromFile
    manager = cls.fromDict(data)
  File "C:/Users/waynew/AppData/Local/Allen Institute/CellLocator 0.1.0-2021-05-21/bin/../lib/CellLocator-4.13/qt-scripted-modules/Home.py", line 407, in fromDict
    manager.annotations = [Annotation.fromDict(item) for item in data['markups']]
KeyError: 'markups'

[allemangD]

Good to know. Sorry that didn't work, but thanks for the traceback. Once it finishes rebuilding I'll test and get back to you with a workaround or make some changes to that PR.

[allemangD]

Do you have a log of Cell Locator creating the problematic file? When I follow the steps you describe, I see the value [] instead of null. The file opens without error this way, so it might be an option to replace null with [] as a workaround.

---

After testing on the same version of Cell Locator, I'm able to load if measurements is missing or []. I'm not able to load if it is null. The error you sent after removing the measurements field indicates to me that something else in the file might have been modified.

Could you try loading these two sample files and see what happens? They should both contain these curve and point annotations out above the ccf atlas, and I'm able to load them both on my machine.

point-test.zip

Thanks for the help in debugging.

[wbwakeman]

I get the same thing trying it now with my version. I'm checking in with the stakeholder who initially reported this to me.

[ru57y34nn]

@allemangD I am the stakeholder that reported this issue to Wayne. I have not found any log files from the Cell Locator but I would be willing to do a Zoom or Teams call and screenshare with you as I work through some experiments and pin cells with the Cell Locator if that would be helpful for you to troubleshoot this issue. Just let me know, thanks!

[allemangD]

@ru57y34nn That would be helpful. Could you reach out to me at david.allemang@kitware.com and we'll schedule a time? Zoom or Google Meet would be best for me.

@wbwakeman, the new converter I've added to the conversion scripts in https://github.com/BICCN/cell-locator/pull/183 should now be able to fix this kind of issue. That might help to normalize the existing data, although I still need to work with @ru57y34nn to find out the root cause.

I've tested the new converter with the issue described in #182. All converters ignore the "measurements" value as input; the difference is in the output from each converter. Targeting version v0.1.0+2020.09.18 will unconditionally add "measurements": []. Targeting version v0.1.1+2021.06.11 will unconditionally remove the "measurements" key.

[...]

# remove "measurements"
python convert.py -v '0.1.0' -t '0.1.1' 'bad_file.json' 'output.json'

# set "measurements": []
python convert.py -v '0.1.0' -t '0.1.0' 'bad_file.json' 'output.json'

[allemangD]

@jcfr Another approach could be to have Cell Locator check the measurements key during load, and replace any null with []. That doesn't seem optimal, as it's adding a special case and we still wouldn't know know the root cause. However such a change would allow loading these broken files in the meantime. Do you have thoughts?

[jcfr]

Do you have thoughts?

Thanks for the summary :pray: After we have an understanding of the root cause, we will be able to define the best course of action.

[allemangD]

After meeting with @ru57y34nn, I believe the issue may be coming from data validation in the LIMS server. We were able to verify that the installation of Cell Locator on the lab machine is able to save and open files with "measurements": [], but that the JSON returned from the LIMS server contains "measurements": null. We weren't able to verify on the lab machine that the JSON file contents are exactly the same as the request body; however I've tested that on my own machine and indeed they are the same. I think it's unlikely that something on the lab machine is changing that behavior.

I have a hunch that, since the addition of the "measurements" field was unannounced with the upgrade to the underlying Slicer version, it might be caught in some edge case in the validation logic; however I don't know enough to make any detailed guesses. @wbwakeman, do you have any ideas?

The changes in https://github.com/BICCN/cell-locator/pull/183, particularly in commit ed00f58025f741e99b9925dd62e57e0b2963e283, ought to prevent unwanted values from sneaking in like this in the future.

We did have some issues with screen-share on the lab machine; Rusty is planning to schedule another meeting this Friday or early next week so that I can see the workflow in more detail, as I've not yet had a chance to. He mentioned that Ariel may be able to join, since she is more familiar with the backend of LIMS and the workflow engine. @jcfr would you be interested in joining this?

Thanks again to @ru57y34nn for the help troubleshooting!

[wbwakeman]

@ru57y34nn Have you entered a new point annotation for any specimens other than those with ids in this list? 1116672992 1109461725 1109460686 1109462043 1109460927 1109460604

These are specimens for which I see current specimen_metadata records in LIMS with 'Point' annotations containing "measurements": null (the last one as recent as today - maybe part of your troubleshooting session). What is the version of Cell Locator that is running on the lab machine?

[wbwakeman]

I was able to reproduce that LIMS is changing "measurements": [ ] into "measurements": null when it is POSTed to LIMS. Checking with the LIMS devs about why that would be.

And here is why according to Ruby on Rails:

Value for params[:data][:markups][:markup][:measurements] was set to nil, because it was one of [], [null] or [null, null, ...]. 
Go to http://guides.rubyonrails.org/security.html#unsafe-query-generation for more information.

@allemangD With this Rails security measure in mind, does the merged PR #183 remove measurements key altogether anyway? If so, is it possible to get a new Cell Locator release with that change?

[allemangD]

@wbwakeman that's good to know. Thanks for digging up the root cause!

183 removes measurements from the saving logic, however it doesn't modify the loading logic at all. The files which already have measurements: null would still fail to load; but newly saved files would be fine.

It should™ not be too hard to add similar logic as 0079e5c8f7de369ce1d3fe81e622184c51fee2b9 to Annotation.fromDict which would also allow these broken files to be loaded; however since @ru57y34nn indicated that only dummy files are broken, that might not be necessary.

I'll talk with @jcfr about cutting a new release.

[wbwakeman]

@allemangD I'm not too concerned with "saving" old data. We just need to get it working for new saves.

[wbwakeman]

I've been playing around with this on the production LIMS system. As long as I remove "measurements": null, from the json that was saved into the LIMS database, then the LIMS integration works okay.

[allemangD]

In that case that the change already merged in #183 would be sufficient. And if for some reason backwards compatibility is needed then we now know we can patch the JSON or target -t 0.1.1 with the converter script.

[allemangD]

I would defer to @wbwakeman and @jcfr if we want to cut a new release now, or wait till some more of the issues in https://github.com/BICCN/cell-locator/milestone/3 are resolved.

[wbwakeman]

@allemangD @jcfr Is it possible to estimate how far away a new release would be if we do wait to include updates to the Usability milestone issues?

[allemangD]

@wbwakeman @ru57y34nn The issue is resolved in the Release 0.2.0-2021-08-12. Please do the tests with the this new version.

To reiterate; the new version still cannot load files with "measurements": null. However saved files no longer contain measurements at all. If there are somehow any files that need to be made compatible, simply remove the measurements value entirely.

[ru57y34nn]

Thank you @allemangD! I will try to get this new release tested at some point this week and follow up here afterwards

[ru57y34nn]

@allemangD I tested the latest release of the cell locator and I can confirm that it now loads the previous annotations from LIMS! thanks for getting that fixed, but I have some questions about how the annotations are numbered, especially when removing a point and then adding a new one. Do you have some time next week so we can jump on a Zoom call and I can demonstrate the issue that I am seeing and we can talk about possible solutions?

[allemangD]

Sure. Since this issue is resolved I will close it; we can create a new one for the annotation numbering.

[ru57y34nn]

Sounds good, thanks.