isi-vista / adam

Abduction to Demonstrate an Articulate Machine
MIT License
10 stars 4 forks source link

Frontend live updates #1161

Closed sidharth-sundar closed 2 years ago

sidharth-sundar commented 2 years ago

Closes the live image display portion of #1159

There are 2 major changes present in this update:

I can make some additional improvements around these features, such as presenting the notification of new data more cleanly/more gracefully handling errors with image retrieval.

sidharth-sundar commented 2 years ago

Two structural comments I wanted to bring up while I remembered them

  1. @spigo900 mentioned that we can be stricter with how we have users retrieve files, more in line with the newly implemented poll_changes() function or the get_scene() function. One way we can do this is by setting app-specific environment variables on initiation. When get_all_learners(), get_all_train_curriculum(), and get_all_test_curriculum() are initially invoked, we can maintain a list of learners and curricula, and can later enforce that the user-provided learners/curricula are among the selected ones.
  2. I initially implemented get_image as an API call because it made sense to me for image retrieval to take place independently; however, with the current approach, selecting a scene generates 1 GET request for retrieving image paths, and 1 GET request per individual image. We could instead wrap all of this functionality into a single request, where the get_scene call in Flask invokes get_image within the back end itself.
spigo900 commented 2 years ago

@sidharth-sundar Re: structural issues:

  1. I think it makes sense to be more restrictive here unless @lichtefeld objects. Is this a place where we can reuse LEARNER_DIR_CONTENTS?
  2. I think it makes sense to bundle the images into the initial retrieval -- send everything we've got -- and also have the separate API call so the UI can fetch additional images as they come in.
sidharth-sundar commented 2 years ago

@spigo900 Re: Re: structural issues:

  1. Yes, we could reuse it here.
  2. Bundling images into retrieval is easy to implement, so I can finish that up today. Just one additional comment.

    and also have the separate API call so the UI can fetch additional images as they come in.

Currently the UI is set up to just alert the user if new data is available, and the user would then have to manually retrieve said data/resubmit the request for data. In the original discussion in #1155 we proposed automatically fetching additional images as a possibility, but it wasn't in scope for what I initially planned to work on. However, I think with the approach I took to retrieve data (polling the back end), I can feasibly implement auto-fetching without too much more effort.

spigo900 commented 2 years ago

@sidharth-sundar Re: fetching as they come in, I meant that more as "it's convenient for future work to leave in the API call." If you feel like adding auto fetching, I think it makes sense to spend up to an hour on that. If it would take more than that, I think it makes more sense to prioritize GNN improvements.

spigo900 commented 2 years ago

I would like to see the pure-cleanup changes to get_scene() split out of c7778e847db1317ef066b707050f3dc50f01d855 (change to relpath) and 88742f3893da77f801db2a5c84ee6e07c4c6a6ae (arg validation changes: and->or; any("/" in...); resolve&relative_to check), and ideally combined into one commit that comes before the "new feature" changes. Otherwise this looks good, thanks for the cleanup.

sidharth-sundar commented 2 years ago

@spigo900 Re: cleanup changes Done! I think there's one other thing which could be moved to the initial commit, which is displaying images with the format rgb_#. However, I think it makes sense to leave it where it currently is, coupled with the larger change in get_scene which now returns image data rather than image paths.

spigo900 commented 2 years ago

@sidharth-sundar That sounds fine. Are you ready to merge then?

sidharth-sundar commented 2 years ago

@spigo900 Yes, it's ready to be merged