Initial Dataset Management GUI by robertbartel · Pull Request #197 · NOAA-OWP/DMOD

robertbartel · 2022-10-12T18:52:06Z

No description provided.

christophertubbs · 2022-10-12T19:39:10Z

+        super().__init__(*args, **kwargs)
+
+    def _get_dataset_content_details(self, dataset_name: str):
+        result = asyncio.get_event_loop().run_until_complete(self.dataset_client.get_dataset_content_details(name=dataset_name))


Looks like there are several calls to asyncio.get_event_loop().run_until_complete. It might help to have some synchronous versions of these functions on the dataset client since there appears to be some intention to call them synchronously. Maybe.

The base functions for sending data through websockets are (and I am pretty sure must be) async, as are the functions making sure the client is authenticated (which relies on the former also). It would take a decent bit of changing to be able to do this and have everything work, but we can look more at that in the future.

christophertubbs · 2022-10-12T19:42:12Z

+
+    def get(self, request, *args, **kwargs):
+        request_type = request.GET.get("request_type", None)
+        if request_type == 'download_file':


This if chain leads me to believe that splitting this out across several smaller views might help.

I did originally start to do that, but it occurred to me that this kind of violated the intent of having only a single websocket (at least per authenticated session) open for all client communication. In fairness, that isn't working properly at the moment, but I think we want to try to have it that way eventually, or else we'd move to REST. And if we are eventually going for that, I don't think we want too many different views (i.e., because eventually we'll have to consolidate them), although I could be assessing that incorrectly.

yeah, if there's a plan to consolidate splitting them out doesn't make a whole lot of sense.

christophertubbs · 2022-10-12T19:44:43Z

+            return self._get_dataset_json(dataset_name=request.GET.get("name", None))
+        elif request_type == 'contents':
+            return self._get_dataset_content_details(dataset_name=request.GET.get("name", None))
+        if request_type == 'delete':


This could probably be handled through a delete handler instead of a get handler. I don't know what all's going on in the workflow (like what the authentication looks like), but it looks like a user could possibly delete datasets through a browser address.

Indeed, but that should eventually get protected by our authentication and authorization mechanisms. And then this relates to my other comment about how we (probably) want to consolidate views now, or will eventually, for how we want our websocket communication to work.

christophertubbs · 2022-10-12T19:47:45Z

+            return self._delete_dataset(dataset_name=request.GET.get("name", None))
+
+        # TODO: finish
+        return JsonResponse({}, status=400)


Django has a specialized response that might help here at django.http.HttpResponseBadRequest. If you use it it can help out when diagnosing issues in debug mode.

^^
Related, do we want to provide any context as to why a 400 was the response? i.e. expected one of request_type: .

christophertubbs · 2022-10-12T19:50:58Z

+        errors, warnings, info = extract_log_data(kwargs)
+
+        # Gather map of serialized datasets, keyed by dataset name
+        serial_dataset_map = asyncio.get_event_loop().run_until_complete(self.get_datasets())


Another asyncio.get_event_loop().run_until_complete call. It might help to have two get_datasets - get_datasets and get_datasets_async, with the former just calling the latter with the event loop wrapper.

I guess we can, but that seems cluttered. Not that I haven't already done that kind of cluttering in places ...

Do you think that would just enhance developer usability of the client class, or were there other advantages you were anticipating?

I know that I'm the type of goober that would end up calling self.get_datasets() and forget to use the asyncio logic. That's just about the only thing that the change would address.

christophertubbs · 2022-10-12T20:00:20Z

+            FileWrapper(custom_filelike, chunk_size),
+            content_type="application/octet-stream"
+        )
+        response['Content-Length'] = asyncio.get_event_loop().run_until_complete(self.dataset_client.get_item_size(dataset_name, item_name))


StreamingHttpResponses are a little wonky - I don't think they are compatible with the Content-Length header and will clash with Gunicorn. It might be better to have a websocket that emits this data and let the client build the blob.

I don't have this part working anyway, so it can be implemented differently. I can pull this and other such things out entirely, but I wasn't sure if parts might be useful to show the direction I was trying to go.

That's precisely why it should be left in for now. I just wanted to drop some notes while I was looking at it. I don't think any of us has the bandwidth to make major changes at the moment.

christophertubbs · 2022-10-12T20:12:10Z

+            arguments[0].appendChild(row);
+        }
+
+        function buildDatasetFilesEmptyDiv() {


It might help to define these styles in a css document rather than baking them into the template. They'd be easier to change and help reduce some code bloat.

Indeed, I started down that road for several things but didn't get everything and wanted to push this. I plan to do more clean up, it's just a question of whether it's before or after the review and merge of this.

I think we will be able to remove a lot of this if / when we move to css grid / flexbox. I think we are all on the same page though.

christophertubbs · 2022-10-12T20:15:40Z

+            });
+        }
+
+        function ajaxBuildDatasetFilesTable(dataset_name, contents_json, table) {


It might be easier to take advantage of a template tag here instead of programmatic element creation. That'd help reduce possible nesting errors and make things a little easier to read (since you'd be reading markup instead of js).

christophertubbs · 2022-10-12T20:20:12Z

+        */
+    </script>
+    <style>
+        #header {


We should probably consolidate these and styles from other templates into a common css document to help maintain consistency between pages. Branding, for instance, could be used on just about every page, for example.

christophertubbs · 2022-10-12T20:23:02Z

@@ -0,0 +1,797 @@
+<!DOCTYPE html>


Templates can inherit from base templates and override/fill out different elements from the parent. Using a base template here could probably cut down the size of this template down by a large portion and make it so the only things that need to be written here are the things specific to dataset management and dataset management alone.

This and we can share common things like stylesheets.

christophertubbs · 2022-10-12T20:24:43Z

+    </style>
+</head>
+<body onload="initGuiViews()">
+<div id="container">


This sort of boilerplate could be farmed out to a base template so you don't have to worry about all that here.

robertbartel · 2022-10-19T16:54:05Z

With PRs #197 and #199, I believe once one of them is merged, the other will need modifications to resolve conflicts.

christophertubbs · 2022-10-20T12:53:08Z

I approved #199, so this will need to be modified.

aaraney · 2022-10-20T13:40:16Z

+from typing import Dict
+
+
+class AbstractDatasetView(View, DMODMixin, ABC):


I get the desire to add blanket implementation here for downstream classes, but given that all methods and properties are implemented, is the use of abstract here to indicate to downstream users that they should inherit from this? Still coming up the speed on the codebase so this might be an idiom ive not picked up on yet.

Essentially yes. In particular, to reusably abstract a view that has a dataset client property. But it's not especially meaningful on it's own.

aaraney · 2022-10-20T13:42:16Z

 from pathlib import Path
 from typing import List, Optional, Tuple, Type

+GUI_STATIC_SSL_DIR = Path('/usr/maas_portal/ssl')


we might want to mark this as a TODO in the future to make this configurable.

I think this should also be handled by Django as well. Shouldn't need to be too necessary. Should be in settings.py

Nice! Thanks for catching this. A comment noting this implicit behavior would be appreciated!

For some reason I thought I put that in settings.py.

aaraney · 2022-10-20T18:03:07Z

+
+    def read(self, blksize: int) -> AnyStr:
+
+        result = asyncio.get_event_loop().run_until_complete(


Do we want to try catch here? If the download_item_block call raises an exception and a downstream user of DatasetFileWebsocketFilelike catches an exception, the state of their DatasetFileWebsocketFilelike could be corrupted or at least unclear what the behavior should be. Meaning, here, _read_index, if caught during a failed read, should continue from the failure point. Is this the expected behavior, or should _read_index be reset to 0?

aaraney · 2022-10-24T17:18:04Z

+    updateFormatChange(selection) {
+        let dy_div = document.getElementById(this.dynamicVarsDivId);
+        //let initial_length = dy_div.childNodes.length;
+        dy_div.childNodes.forEach(child => child.remove());


Related to #198. See this comment specifically. This should be changed to the following:

// remove all dynamic input child elements while (dy_div.firstChild) { dy_div.removeChild(dy_div.firstChild); }

aaraney · 2022-10-24T19:35:25Z

+        super().__init__(*args, **kwargs)
+
+    def _get_dataset_content_details(self, dataset_name: str):
+        result = asyncio.get_event_loop().run_until_complete(self.dataset_client.get_dataset_content_details(name=dataset_name))


It seems we might want to wrap these methods (all methods with leading _) with try except's and return something meaningful to the client if the request fails. I didn't go looking, but if the result has something like an ok property, we might want to return a non-200 here.

aaraney · 2022-10-24T19:41:12Z

+
+        # Gather map of serialized datasets, keyed by dataset name
+        serial_dataset_map = asyncio.get_event_loop().run_until_complete(self.get_datasets())
+        serial_dataset_list = [serial_dataset_map[d] for d in serial_dataset_map]


this is nit-picky, but for code readability, serial_dataset_list = list( serial_dataset_map.values()) might be preferred.

aaraney · 2022-10-24T19:45:20Z

+        if self.last_response.success:
+            return self.last_response.data
+        else:
+            return {}


we might want to return something to the caller indicating why, if we know, the response was unsuccessful. thoughts?

aaraney · 2022-10-24T19:46:49Z

+        if self.last_response.success:
+            return self.last_response.data
+        else:
+            return -1


aaraney · 2022-10-24T19:48:56Z

+        if self.last_response.success:
+            return self.last_response.data
+        else:
+            return ''


^^ ditto. For non-dict return types, does it make sense to raise an exception here?

robertbartel · 2022-10-31T17:04:43Z

FWIW, I don't particularly understand why tests are (now) failing after rebasing in other upstream changes. The same tests are not failing locally for me or when run on ucs3. Given that, I think we should proceed with this (pending final review approval).

aaraney · 2022-10-31T19:42:12Z

+            Either ``communication.MessageEventType.DATASET_MANAGEMENT`` or
+            ``communication.MessageEventType.DATA_TRANSMISSION``.
+        """
+        # TODO:


Issue to track this? Or will this be completed in this PR?

aaraney · 2022-10-31T19:46:09Z

+from .AbstractDatasetView import AbstractDatasetView
+
+
+class DatasetManagementView(AbstractDatasetView):


Thoughts on adding a delete method here to handle deleting a dataset? Or will that fall under the responsibility of post? Meaning, post would then likely handle creation, deletion, and modification.

Updating service to respond to GET_DATASET_ITEMS queries and requests for data with an offset start (i.e. partials).

Adding initial second view display and manage existing datasets, along with navigation functionality to toggle between "manage" and "create" views.

Sending serialized datasets to HTML template as list/array rather than dict/map.

Implementing layout and initial details-viewing behavior for dataset management GUI view.

Adding commented-out line for host bind-mount volume to GUI Docker service config to make debugging Django template and Javascript code faster by mounted code from the host into the running service.

Make DatasetManagementView inherit from AbstractDatasetView so certain required pieces could be centrally located and reused.

Adding several classes used by Dataset management GUI view.

Cleanup, a bit of additional behavior, and attempts at getting downloads working.

Adding incomplete DatasetFileWebsocketFilelike class, since it is referenced by other things.

Fixing issue from some upstream changes where numpy was still required but no longer included implicitly/transitively as a dependency.

Fixing issue from some upstream changes where migrations were necessary for dataset management GUI to function (even though they weren't used there) but were not run automatically.

robertbartel · 2023-01-11T14:40:39Z

Closing; consolidating these and other changes in #240.

christophertubbs reviewed Oct 12, 2022

View reviewed changes

robertbartel force-pushed the e21/dataset_mgr_gui/main branch from 6423883 to b52cfd7 Compare October 17, 2022 15:48

This was referenced Oct 19, 2022

Update Dataset Creation Form For Dynamic Fields #198

Closed

Move Dataset classes to dmod.core package #199

Merged

aaraney reviewed Oct 20, 2022

View reviewed changes

aaraney reviewed Oct 24, 2022

View reviewed changes

aaraney mentioned this pull request Oct 28, 2022

Dynamic Dataset creation form #210

Closed

robertbartel force-pushed the e21/dataset_mgr_gui/main branch from b52cfd7 to 0f3aeb4 Compare October 31, 2022 16:36

aaraney reviewed Oct 31, 2022

View reviewed changes

robertbartel added 26 commits November 23, 2022 12:33

Update client unit tests for interface changes.

0f0cd00

Bump client package version to 0.2.0.

fc13804

Update dataservice response handling.

2afa9bd

Updating service to respond to GET_DATASET_ITEMS queries and requests for data with an offset start (i.e. partials).

Update dataservice package dependency versions.

edd2622

Add dataset view class and static page for GUI.

0d0ad3a

Refactor static dir usage in DMODProxy.py.

a6b0aa6

Add Django GUI url def for dataset view.

354d3c5

Add client package build arg for GUI Docker build.

9916b4d

Add second view and navigation.

14f675f

Adding initial second view display and manage existing datasets, along with navigation functionality to toggle between "manage" and "create" views.

Have DatasetManagementView.py send DS as list.

57632b1

Sending serialized datasets to HTML template as list/array rather than dict/map.

Update dataset management GUI with details view.

cf2256a

Implementing layout and initial details-viewing behavior for dataset management GUI view.

Cleanup after changing implementation approach.

ced58ed

Create AbstractDatasetView GUI Django view class.

09afa58

Add Django view for direct dataservice API calls.

5f0384d

Add GUI Django url for Dataset API AJAX calls.

45303a9

Add commented-out helper debug volume for GUI.

e98ef36

Adding commented-out line for host bind-mount volume to GUI Docker service config to make debugging Django template and Javascript code faster by mounted code from the host into the running service.

Refactor DatasetManagementView inheritance.

3a7e39f

Make DatasetManagementView inherit from AbstractDatasetView so certain required pieces could be centrally located and reused.

More progress on dataset management template.

2c44f7e

Add dataset delete support to Django API view.

ea10c34

Implement dataset delete in management view.

9717569

Adding Javascript component helper classes.

8ce767f

Adding several classes used by Dataset management GUI view.

Update dataset API view.

e47e730

Update dataset management view template.

9831d00

Cleanup, a bit of additional behavior, and attempts at getting downloads working.

Add incomplete DatasetFileWebsocketFilelike.

7d81da7

Adding incomplete DatasetFileWebsocketFilelike class, since it is referenced by other things.

Fix GUI numpy dep issue from upstream changes.

953cfdd

Fixing issue from some upstream changes where numpy was still required but no longer included implicitly/transitively as a dependency.

Fix GUI migrations issue from upstream changes.

508a0ba

Fixing issue from some upstream changes where migrations were necessary for dataset management GUI to function (even though they weren't used there) but were not run automatically.

robertbartel force-pushed the e21/dataset_mgr_gui/main branch from 3d0abaf to 508a0ba Compare November 23, 2022 18:33

robertbartel mentioned this pull request Jan 11, 2023

Combined pre-AGU GUI and Calibration Message Work #240

Open

robertbartel closed this Jan 11, 2023

		from typing import Dict


		class AbstractDatasetView(View, DMODMixin, ABC):


		def read(self, blksize: int) -> AnyStr:

		result = asyncio.get_event_loop().run_until_complete(

		from .AbstractDatasetView import AbstractDatasetView


		class DatasetManagementView(AbstractDatasetView):

Conversation

robertbartel commented Oct 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbartel commented Oct 19, 2022

Uh oh!

christophertubbs commented Oct 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaraney Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaraney Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aaraney Oct 20, 2022 •

edited

Loading

aaraney Oct 24, 2022 •

edited

Loading

aaraney Oct 24, 2022 •

edited

Loading