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

merges web kernel framework with mast html page #19

Closed
wants to merge 24 commits into from
Closed

merges web kernel framework with mast html page #19

wants to merge 24 commits into from

Conversation

havok2063
Copy link
Collaborator

This PR addresses #12 and is based off of Nick's #7 PR. This loads a glue image viewer of the manga cube into a div on the html page and includes an html table with pre-populated content.

The original three cases to explore from #12 were

  1. click something in the widget to inject information into the html table
  2. click web element (button or table data) to manipulate the widget and/or inject information into it
  3. click web element to make a "public astroquery" call to manipulate/update the widget content

Current Status

  • Item 1 is not complete. There is no clear way to trigger front-end events based on widget events. For example, I'd like to inject data into the html table when a data subset is created.
  • Item 2 is half complete. The Get Info from Widget button extracts information from the data loaded in the widget and displays it in the html element Output Area. The Overlay Row Data on Widget takes the selected table rows and overplots them as a new dataset in the widget. This code currently works in the notebook but not in the browser. There is an error inside the widget rendering code preventing the points from being drawn on the image. However the new dataset is properly being created inside glue and shows up in the widget tab area. The error that shows up is
Loading class bqplot.
only types uint8 and float32 are supported

which suggests a possible type/format mismatch. The error roughly occurs in

update_image @ index.built.js:377712		
render	@ index.built.js:377580		
ManagerBase.create_view	@ index.built.js:257940
WidgetView.create_child_view @ index.built.js:98033
add_mark @	index.built.js:155111
ViewList.update @ index.built.js:302668
triggerEvents	@ index.built.js:130832
...
  • Item 3 is complete. The Update Widget button makes a call to astroquery to download an HST image and replaces the current widget with a new one based off that image.

@havok2063 havok2063 requested a review from nmearl May 24, 2019 16:07
@nmearl
Copy link
Contributor

nmearl commented May 29, 2019

Great job. Some immediate issues that jump out at me:

  1. This shouldn't be messing with the index.html file -- whatever mast front-end there is should be represented by its own html file and be served via its own web server. It should just include the index.built.js file in its src.
  2. The entire widget is being rebuilt in the embedded javascript added to the index.html file. This should not be -- the webapp should not be being rebuilt over and over, only altered.

We want to make the app as separable as possible, meaning it should only end up being a single access point -- the .js file -- and should only be interacted with through a single variable: window.Manager.kernel.

I'm going to try and rearrange this.

@havok2063
Copy link
Collaborator Author

Ok, thanks for taking a look. Comments on your comments

  1. I was under the impression the multi-server option was out of scope for this prototype. This was meant to just get something up and running. We definitely need to explore the prototype capabilities in the context of the multi-server solution.
  2. I'm not quite sure what you're referring to here. If you mean the feature where I destroy the widget and make a new one on the Update Widget button, I think this is a realistic option, e.g. loading a new type of viewer in the same div space.

But please feel free to do what you need to do to get things working for the demo.

@nmearl
Copy link
Contributor

nmearl commented May 29, 2019

You're absolutely correct: this is more than sufficient for the demo and shows off exactly what we're after -- no doubt. I just mean that we're not going to be able to merge this into web because it co-ops the standalone webpack/electron rendering. In the future, we'll have to parse it out to its own mast app that will just have a dependency on the *viz apps.

I think this is a realistic option

For now, it's fine, because the mast web app isn't using cubeviz/mosviz directly. But when that time comes, viewers should be removed/added through the application, without destroying the entire rendered widget. E.g. we already have the ability in cubeviz to add more viewers, and in the future, the option to remove a viewer, without messing with the entire state of the app. Does that make a bit more sense?

@brechmos-stsci
Copy link
Contributor

@havok2063, generally is this ready for merging? If so, could you rebase and fix the conflict?

@nmearl
Copy link
Contributor

nmearl commented Jun 13, 2019

This cannot be merged since it changes fundamental workings of the web interface for use in the standalone web app and the electron app. This was only for demonstration purposes for the communication between a "mast" front-end and the running jupyter kernel. @havok2063 is working on a new implementation using the mast microservice to host an independent mast site with the app embedded.

@havok2063
Copy link
Collaborator Author

havok2063 commented Jun 18, 2019

This PR has now been deprecated and no longer needs to be merged. The majority of this work has been separated out into a standalone ASB web server, in a grit.stsci.edu repo. The work in the new repo, based off this one, effectively addresses #12 (JIRA 86). Looking at the JIRA issues, I think it resolves issues 60, 61, 62 albeit in a hacky way, 64, 65.

@nmearl nmearl closed this Jun 8, 2020
rosteen pushed a commit that referenced this pull request Sep 16, 2021
james-trayford pushed a commit to james-trayford/jdaviz that referenced this pull request Nov 8, 2024
…n-updates-jt

Some more functionality tweaks
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.

3 participants