Improve dmod.client and update CLI #424

robertbartel · 2023-09-08T19:44:43Z

General improvements to the CLI, as well as updates to bring in line with more recent DMOD updates to serialization, communication client classes, and user config representation.

Note that this PR depends on #409 and should remain in draft status until that is merged.

aaraney

Thanks @robertbartel! I have a few fairly minor comments that more or less boil down to API parameter choices. There is one bug that will need to be addressed but other than that, this should be pretty easy to get through. Thanks!

python/lib/client/dmod/client/dmod_client.py

python/lib/client/dmod/client/request_clients.py

aaraney · 2023-09-14T18:25:25Z

python/lib/client/dmod/client/request_clients.py

-    async def upload_to_dataset(self, dataset_name: str, paths: List[Path]) -> bool:
+        chunk_size = 1024
+
+        with source.open('r') as file:


I think we need to open this in reading bytes, right? Since we dont know what kind of item is being uploaded.

Hmm ... yeah, I believe you are correct, and as such we should do similarly for writes as you suggested above. But this actually becomes a larger undertaking. All the links in the chain will need to be modified to accommodate, from DataTransmitMessage.data to the declaration and implementations of _DatasetManager.add_data()` (and associated helper methods).

It probably makes sense to do this as part of #432. What do you think?

Ping again here also. This probably relates to the earlier item and my old follow-up questions.

Yeah I agree. This is a larger undertaking and not directly related so should be in its own PR.

I opened #486 to track changing DataTransmitMessage's data field to a more appropriate type. Im not sure if there are other models off the top of my head that will also need to be changed.

It didn't occur to me back in September, but (unless I'm missing something) we can't actually do this because it will break JSON serialization.

Yeahhhhh, you are right. I forgot about that. Does that mean you can only upload and download unicode encoded data at this moment? I don't recall seeing any logic to base64 / base85 encode binary data.

Just as a sanity check, if we decode and then encode binary data using the "wrong" encoding, but it's the same encoding on both ends, won't we get the same data we started with?

Assuming more discussion and work is needed, I suggest that we defer this problem. A solution will probably be complex, and we can find workarounds as needed to get datasets into the system (worst case: temporarily opening up MinIO externally and uploading directly).

Just as a sanity check, if we decode and then encode binary data using the "wrong" encoding, but it's the same encoding on both ends, won't we get the same data we started with?

Yeah! You are right. I may be blowing this out of proportion and it might not be a problem after all. I thought to transmit any data to and from the request service (and therefore the data service) it must be in json and therefore in some unicode flavor (e.g. utf-8). So, for example if I wanted to upload a netCDF forcing file to a dataset, I think I would first need to encode and chunk the data using base64 or base85, package that up in json, and send it along, right? I agree that we should move this discussion elsewhere and just focus on getting the PR through lol. Sorry to trying to derail us 😅!

python/lib/client/dmod/client/request_clients.py

aaraney · 2023-09-14T18:55:09Z

python/lib/client/dmod/test/test_dataset_client.py

    """

    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
+        #super().__init__(*args, **kwargs)


Minor: could be removed.

robertbartel · 2024-01-22T18:06:05Z

Trying to get this one moving again after it has sat for a while. I think there are some outstanding follow-up questions, and possible tasks resulting from those discussions, but everything else is settled. @aaraney, when you are able, can you confirm all the other conversations can be marked resolved?

aaraney · 2024-01-23T18:25:29Z

@robertbartel, thanks for revitalizing this! I'd forgotten about it and let it get stale. I've resolved relevant comments and contributed to existing conversation to move this forward. We are getting really close, just a few minor things.

Replacing ngen-job-specific, deprecated client classes with more general JobClient.

Adding more help detail for various subcommands, and renaming some subcommands to be more intuitive.

Replacing pyyaml with Pydantic for ClientConfig changes, and bumping to latest communication package version.

Co-authored-by: Austin Raney <[email protected]>

Fixing bug with misnamed initial variable usage for setting up first message to send before look-ahead sending loop.

Having usage wrap raised exceptions and raise new ones that include the message of the former, rather than crudely pass the name of the user function to DataServiceClient._process_request().

Rename get_dataset_items method in DataServiceClient to get_dataset_item_names.

Fixing mismatched strings between CLI prepared args and supported action strings in DmodClient data_service_action() func.

Commenting out seemingly unneeded SchedulerClient init in RequestService.

Ensuring ConnectionContextClient async_recv() uses an async context manager.

Adding support to package __main__ routine for turning on remote debugging when properly configured.

Re-add DEFAULT_CLIENT_CONFIG_BASENAME definition (that was still being used) and refactor an error message.

Updating Dockerfile for app_server image to create a client config file using the new JSON syntax, complete with optional debugging config if env is set appropriately.

Fixing methods involved in making requests and processing responses to support collections of response types, rather than only a single, to account for scenarios like data transfer.

Updating classes to properly use async context management in transport client when appropriate.

robertbartel · 2024-01-24T19:58:57Z

The only thing different was manually resolving a conflict for a version bump to the communication package, and the result of that ended up the same. As such, I'm going to bypass waiting for another approval this time.

robertbartel added enhancement New feature or request maas MaaS Workstream labels Sep 8, 2023

robertbartel requested review from christophertubbs and aaraney September 8, 2023 19:44

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch from 6f55b6d to 8d29ffe Compare September 8, 2023 20:34

robertbartel mentioned this pull request Sep 11, 2023

Update DMOD CLI for calibration job execution support #264

Closed

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch from 8d29ffe to 5caf4a3 Compare September 12, 2023 18:23

robertbartel marked this pull request as ready for review September 12, 2023 18:23

aaraney requested changes Sep 14, 2023

View reviewed changes

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch from 5caf4a3 to f39d89e Compare September 18, 2023 18:59

This was referenced Sep 18, 2023

Complete implementations of Request Client functions #431

Closed

Develop and incorporate data access protocol/client classes #432

Open

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch 2 times, most recently from 3c08c84 to a1d10d3 Compare September 20, 2023 14:11

robertbartel mentioned this pull request Sep 21, 2023

Add and improve automated unit tests for classes in dmod.client #438

Open

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch 2 times, most recently from 48cb083 to 0ac3a65 Compare October 4, 2023 12:41

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch 2 times, most recently from dca9d6d to 2b38710 Compare October 31, 2023 20:03

robertbartel mentioned this pull request Nov 2, 2023

Refactor Service Packages to Use Config Files Instead of CLI Arguments #462

Open

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch from 3c431ff to e0c9ed9 Compare November 2, 2023 13:12

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch from cd66f76 to c0bec91 Compare January 22, 2024 17:53

This was referenced Jan 23, 2024

bug(py38): port functools.cache support #483

Merged

Address issues related to DataTransmitMessage's data field type and handling of binary data #486

Closed

aaraney mentioned this pull request Jan 23, 2024

ModelExecRequestClient removed in #409, but still used in several places #467

Open

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch from c0bec91 to 8e6e3bd Compare January 24, 2024 18:35

aaraney previously approved these changes Jan 24, 2024

View reviewed changes

General JobClient for requests.

7f2c2a9

Replacing ngen-job-specific, deprecated client classes with more general JobClient.

robertbartel and others added 24 commits January 24, 2024 14:55

Fix unit tests for [Dataset/DataService]Client.

036cdc0

Add simply unit test for DmodClient.

dfd1759

Improve dmod.client main func behavior.

5812764

Adding more help detail for various subcommands, and renaming some subcommands to be more intuitive.

Update dmod.client package dependencies.

7b9f1b7

Replacing pyyaml with Pydantic for ClientConfig changes, and bumping to latest communication package version.

Bump dmod.client package version to 0.4.0.

5513678

Safeguard write to disk when loop through transfer recpts

2fa6831

Co-authored-by: Austin Raney <[email protected]>

Fix bug in SimpleDataTransferAgent upload.

013cac9

Fixing bug with misnamed initial variable usage for setting up first message to send before look-ahead sending loop.

Refactor exception handling for _process_request.

ea2d946

Having usage wrap raised exceptions and raise new ones that include the message of the former, rather than crudely pass the name of the user function to DataServiceClient._process_request().

Rename func to get_dataset_item_names.

f79ccea

Rename get_dataset_items method in DataServiceClient to get_dataset_item_names.

Fix mismatched data service action strings.

8afbc9a

Fixing mismatched strings between CLI prepared args and supported action strings in DmodClient data_service_action() func.

Comment out SchedulerClient init.

8fe5e41

Commenting out seemingly unneeded SchedulerClient init in RequestService.

Fix bug with non-async context manager.

95bed86

Ensuring ConnectionContextClient async_recv() uses an async context manager.

Optional remote debug info in client config.

79d70b2

Support remote debugging on client main().

8b510d3

Adding support to package __main__ routine for turning on remote debugging when properly configured.

Fixes in client package __main__.py.

cd314cb

Re-add DEFAULT_CLIENT_CONFIG_BASENAME definition (that was still being used) and refactor an error message.

Change GUI Dockerfile to new style client config.

c232cd4

Updating Dockerfile for app_server image to create a client config file using the new JSON syntax, complete with optional debugging config if env is set appropriately.

Fix GUI image request service SSL path.

087a5dc

Fix RequestClient to handle multiple response types.

48a6443

Fixing methods involved in making requests and processing responses to support collections of response types, rather than only a single, to account for scenarios like data transfer.

Add service debug env flags to example.env.

8e159b8

Add debug args from env to GUI service image build.

fd90869

Adjust GUI service image to fix client config debug.

9e65c6b

Fix DataRequestHandler after recent client changes.

fc77675

Fix client package clients to use connection context.

a7bfb31

Updating classes to properly use async context management in transport client when appropriate.

Add CLI client arg to turn debugging on/off.

51f583d

robertbartel dismissed aaraney’s stale review via 51f583d January 24, 2024 19:56

robertbartel force-pushed the f/cli_updates/client_pkg_improvements branch from 8e6e3bd to 51f583d Compare January 24, 2024 19:56

robertbartel merged commit 406a9e5 into NOAA-OWP:master Jan 24, 2024
0 of 4 checks passed

robertbartel deleted the f/cli_updates/client_pkg_improvements branch January 24, 2024 20:10

aaraney mentioned this pull request Jan 24, 2024

SchedulerClient tests failing after introducing #424 #489

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve dmod.client and update CLI #424

Improve dmod.client and update CLI #424

robertbartel commented Sep 8, 2023

aaraney left a comment

aaraney Sep 14, 2023

robertbartel Sep 19, 2023

robertbartel Jan 22, 2024

aaraney Jan 23, 2024

robertbartel Jan 23, 2024

aaraney Jan 23, 2024

robertbartel Jan 24, 2024

aaraney Jan 24, 2024

aaraney Sep 14, 2023

robertbartel commented Jan 22, 2024

aaraney commented Jan 23, 2024

robertbartel commented Jan 24, 2024

Improve dmod.client and update CLI #424

Improve dmod.client and update CLI #424

Conversation

robertbartel commented Sep 8, 2023

aaraney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbartel commented Jan 22, 2024

aaraney commented Jan 23, 2024

robertbartel commented Jan 24, 2024