BUG: Handle item is None in certain conditions #1649

pllim · 2022-09-13T21:05:19Z

Description

This pull request is to not let Imviz crash in certain use cases. I am not sure how to write unit test for this.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-09-13T21:30:23Z

Codecov Report

Base: 86.75% // Head: 86.76% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (5be2035) compared to base (c3940e1).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1649      +/-   ##
==========================================
+ Coverage   86.75%   86.76%   +0.01%     
==========================================
  Files          95       95              
  Lines        9639     9652      +13     
==========================================
+ Hits         8362     8375      +13     
  Misses       1277     1277

Impacted Files	Coverage Δ
jdaviz/configs/default/plugins/viewers.py	`95.65% <100.00%> (+0.07%)`	⬆️
jdaviz/configs/imviz/plugins/parsers.py	`100.00% <100.00%> (ø)`
jdaviz/core/helpers.py	`67.88% <100.00%> (+1.21%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kecnry · 2022-09-15T16:29:19Z

jdaviz/configs/default/plugins/viewers.py

+        if viewer_item is None:
+            return


this gets triggered when the viewer is removed from the app before the layer update messages are processed? If so, I think this makes sense as a good way to quickly prevent that case from causing issues. We don't need to worry about any of the following logic (which only affects things in the viewer, not higher in the stack) since the viewer is no longer in the app.

This one is for #1614 . I don't grok all the callback stuff but when this happens:

114 viewer_item = self.jdaviz_app._viewer_item_by_id(self.reference_id) --> 115 selected_data_items = viewer_item['selected_data_items']

self.reference_id actually points to the deleted viewer ID (not sure why, maybe upstream bug). So I think returning here is good enough until we find a better fix.

kecnry · 2022-09-15T16:31:15Z

jdaviz/app.py

+            if data_item is None:
+                label = None


what scenario triggers this case? I'm just trying to think if there's a way we can prevent it entirely, or if not if the warning that it will raise is warranted or will be to noisy.

For this one, I cannot reproduce it, so I just looked at the traceback at #689 (comment) and did my best guess.

ok, at least its not a common scenario then. I don't think the extra logic to prevent an error is a problem... as long as it doesn't result in a lot of warnings for the user. But I guess we'll at least have to reproduce (or find someone who can) before closing that issue. I'll try to do some testing before approving, but otherwise the code changes themselves look good to me! Thanks!

kecnry

I was able to reproduce #689 on main. This PR does prevent the traceback, but I still don't think the workflow in #689 is handled correctly. My vote would be to merge this PR as-is but NOT close that issue (which I'll update with what I'm seeing and how to reproduce) with this PR.

(Regression test coverage would be nice, but I'm not sure if API calls to remove viewers would have the signals processed in the same order to reproduce the traceback or not... it might be worth a shot though!)

pllim · 2022-09-16T20:31:56Z

@kecnry , you mean regression test for #1614?

kecnry · 2022-09-16T20:35:37Z

I suppose both really (coverage for the new lines of code to ensure they avoid those tracebacks)

pllim · 2022-09-16T22:32:33Z

@kecnry , so I think only 2 remaining things:

I need a workflow if you want me to add a test for Imviz: Unable to reload same file multiple times #689. I already added one for delete subset traceback after removing viewer #1614 here.
I also added logic to handle Imviz: Unable to reload same file multiple times #689 (comment) . With this addition, would this PR now actually close that issue or you still want to keep it open?

kecnry · 2022-09-19T12:22:14Z

@pllim - is the workflow I added in #689 not sufficient for the test? It looks like your change to appropriately handle the index does fix the original problem - I wonder if the if data_item is None check should now be removed so that if any other bug uncovers a similar situation it won't silently be ignored and difficult to track down? Or do you think its better to leave that safeguard in to avoid tracebacks for the user?

pllim · 2022-09-19T12:24:18Z

if the if data_item is None check should now be removed

Hmm... If it is not needed for this fix, I guess we can remove it. Lemme see.

now creates a duplicate every time instead of overwriting the second one. This is different from original 0th order fix for spacetelescope#600

which I confirmed that it fails without this patch. Mark line to be excluded from bandit check. Update result for affected remote test.

so the error will crop back up for other cases

pllim · 2022-09-19T15:31:50Z

is the workflow I added in #689 not sufficient for the test?

I think there are 2 different problems being discussed at Issue 689.

I fixed the second duplicate label being overwritten and that is just label generation and should be already tested in this PR via test_helpers.py changes.
I never was able to reproduce the traceback reported at Imviz: Unable to reload same file multiple times #689 (comment)

rosteen

Looks good to me, I confirmed that both of the linked bugs are fixed by this. Thanks!

pllim · 2022-09-19T17:35:57Z

Thanks for the reviews!

pllim added bug Something isn't working imviz labels Sep 13, 2022

pllim added this to the 2.11 milestone Sep 13, 2022

pllim requested a review from camipacifici September 13, 2022 21:05

pllim requested review from duytnguyendtn, rosteen, javerbukh, ojustino and kecnry as code owners September 13, 2022 21:05

kecnry reviewed Sep 15, 2022

View reviewed changes

kecnry approved these changes Sep 16, 2022

View reviewed changes

kecnry added the Ready for final review label Sep 16, 2022

kecnry mentioned this pull request Sep 16, 2022

Imviz: Unable to reload same file multiple times #689

Closed

pllim force-pushed the no-crashing-no-cry branch from 50f7dba to 1bd3d38 Compare September 16, 2022 21:43

This comment was marked as outdated.

Sign in to view

pllim force-pushed the no-crashing-no-cry branch from 7bf7a68 to 54e6b3e Compare September 16, 2022 22:44

pllim added 4 commits September 19, 2022 11:19

BUG: Handle item is None in certain conditions

672abbd

Loading same data multiple times

49659c8

now creates a duplicate every time instead of overwriting the second one. This is different from original 0th order fix for spacetelescope#600

Regression test for spacetelescope#1614

46d5ef9

which I confirmed that it fails without this patch. Mark line to be excluded from bandit check. Update result for affected remote test.

Undo app.py patch

5be2035

so the error will crop back up for other cases

pllim force-pushed the no-crashing-no-cry branch from 54e6b3e to 5be2035 Compare September 19, 2022 15:27

rosteen approved these changes Sep 19, 2022

View reviewed changes

pllim merged commit 1639526 into spacetelescope:main Sep 19, 2022

pllim deleted the no-crashing-no-cry branch September 19, 2022 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Handle item is None in certain conditions #1649

BUG: Handle item is None in certain conditions #1649

pllim commented Sep 13, 2022 •

edited

Loading

codecov bot commented Sep 13, 2022 •

edited

Loading

kecnry Sep 15, 2022

pllim Sep 15, 2022

kecnry Sep 15, 2022

pllim Sep 15, 2022

kecnry Sep 15, 2022

kecnry left a comment •

edited

Loading

pllim commented Sep 16, 2022

kecnry commented Sep 16, 2022

This comment was marked as outdated.

pllim commented Sep 16, 2022

kecnry commented Sep 19, 2022

pllim commented Sep 19, 2022

pllim commented Sep 19, 2022

rosteen left a comment

pllim commented Sep 19, 2022

BUG: Handle item is None in certain conditions #1649

BUG: Handle item is None in certain conditions #1649

Conversation

pllim commented Sep 13, 2022 • edited Loading

Description

Checklist for package maintainer(s)

codecov bot commented Sep 13, 2022 • edited Loading

Codecov Report

kecnry Sep 15, 2022

Choose a reason for hiding this comment

pllim Sep 15, 2022

Choose a reason for hiding this comment

kecnry Sep 15, 2022

Choose a reason for hiding this comment

pllim Sep 15, 2022

Choose a reason for hiding this comment

kecnry Sep 15, 2022

Choose a reason for hiding this comment

kecnry left a comment • edited Loading

Choose a reason for hiding this comment

pllim commented Sep 16, 2022

kecnry commented Sep 16, 2022

This comment was marked as outdated.

pllim commented Sep 16, 2022

kecnry commented Sep 19, 2022

pllim commented Sep 19, 2022

pllim commented Sep 19, 2022

rosteen left a comment

Choose a reason for hiding this comment

pllim commented Sep 19, 2022

pllim commented Sep 13, 2022 •

edited

Loading

codecov bot commented Sep 13, 2022 •

edited

Loading

kecnry left a comment •

edited

Loading