Skip to content
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

FIX Improve robustness of attribute query for JSProxy #456

Merged
merged 22 commits into from
Jul 12, 2022

Conversation

mvdoc
Copy link
Contributor

@mvdoc mvdoc commented Jul 11, 2022

Functions like code.export.plot_panels fail from time to time in an unpredictable way. It looks like the culprit is that the JSProxy object tries to get some attributes that do not exist yet (see #449). However, when inspecting the JavaScript console, those attributes do exist. This suggests that the querying process is not robust enough to random delays in updating the viewer/JS environment.

This PR adds a simple logic that queries for the desired attr up to 10s before giving up. It should make the querying process a bit more robust.

Closes #449 (hopefully for good)

@mvdoc mvdoc requested review from marklescroart and TomDLT July 11, 2022 19:09
Copy link
Contributor

@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

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

I don't quite understand why accessing self.attrs starts a query.

But the PR looks good to me.

(Failing tests are unrelated. They are due to a deprecated function in nibabel 4.0.)

@mvdoc
Copy link
Contributor Author

mvdoc commented Jul 11, 2022

From my reading of the code, JSProxy.__getattr__ implements a special case for JSProxy.attrs, which runs self.send(method='query', params=[self.name])[0]. So every time self.attrs is accessed, a query is sent. This should be fine if, once an attribute is present in self.attrs, subsequent calls to self.attrs still contain that attribute. But I'm not sure this is the case (and some random failures indicated that it wasn't), so I wanted to be careful.

def __getattr__(self, attr):
if attr == 'attrs':
return self.send(method='query', params=[self.name])[0]
if attr not in self.attrs:
raise KeyError(f"Attribute '{attr}' not found in {self}")
if self.attrs[attr][0] in ["object", "function"]:
return JSProxy(self.send, "%s.%s"%(self.name, attr))
else:
return self.attrs[attr][1]

@TomDLT
Copy link
Contributor

TomDLT commented Jul 11, 2022

From my reading of the code, JSProxy.__getattr__ implements a special case for JSProxy.attrs.

Oh right, I missed that for some reason, thanks!

…bglview

* 'fix/webglview' of github.com:mvdoc/pycortex:
  Increase wait time
  MNT remove unused code
  FIX avoid querying too many times
  Missed one line
  ENH try to add waiting time directly to JSProxy
  FIX reuse the surface obj
  FIX deal with keyerror in how __getattr__ is defined
  FIX generalize how to get an attr from self.ui
  Better print message
  Attempt to fix JS UI errors
  FIX update nibabel deprecated methods (gallantlab#457)
@mvdoc
Copy link
Contributor Author

mvdoc commented Jul 11, 2022

Unfortunately, the viewer still crashes from time to time. It seems to me that 10s should be enough time to wait, so maybe the problem is somewhere else. :\

@TomDLT
Copy link
Contributor

TomDLT commented Jul 11, 2022

Does it crash after the 10sec wait?

@mvdoc
Copy link
Contributor Author

mvdoc commented Jul 11, 2022

Yep, it crashes with the same error: (Attribute 'surface' not found in <JS: window.viewer.ui>)...

@mvdoc
Copy link
Contributor Author

mvdoc commented Jul 12, 2022

This new change I implemented seems to have solved the problem (🤞). I made a new property for JSProxy.attrs in case there is some problem with calling __getattr__ recursively in an async setup like we the one we have with tornado and JS. This change also helped me figure out that sometimes querying window.viewer can return None or an int (an error number?), probably when the JS viewer hasn't been updated.

@mvdoc mvdoc changed the title FIX Try getting the attr from JSProxy objects for a max of 10s before failing FIX Improve robustness of attribute query for JSProxy Jul 12, 2022
@mvdoc mvdoc merged commit cd45d2e into gallantlab:main Jul 12, 2022
@mvdoc mvdoc deleted the fix/webglview branch July 12, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

save_3d_views crashes because javascript's window.viewer.ui doesn't have required attrs
2 participants