Data Service upload issues

[aaraney]

https://github.com/NOAA-OWP/DMOD/blame/e5aa5d50837bc751ede138ea213582a3ae173308/python/lib/core/dmod/core/dataset.py#L530

The addition of the domain parameter to the DatasetManager's add_data method in #608 results in both the websocket and rest dataset upload handlers to fail. Looks like I missed the impact of this change in my review of #608.

@robertbartel, can we weaken this requirement to an optional argument?

Code affected:

• DMOD/python/services/dataservice/dmod/dataservice/service.py

  Line 257 in e5aa5d5

  elif manager.add_data(dataset_name=dataset_name, dest=dest_item_name, data=message.data.encode(), is_temp=is_temp):

• DMOD/python/services/dataservice/dmod/dataservice/rest_service.py

  Line 207 in e5aa5d5

  success = service_manager.manager(DatasetType.OBJECT_STORE).add_data(

• DMOD/python/services/dataservice/dmod/dataservice/initial_data_adder_impl.py

  Line 67 in e5aa5d5

  if not self._dataset_manager.add_data(dataset_name=self._dataset_name, dest=dest, data=path.read_bytes()):

• DMOD/python/services/dataservice/dmod/dataservice/initial_data_adder_impl.py

  Line 106 in e5aa5d5

  if not self._dataset_manager.add_data(dataset_name=self._dataset_name, dest=item, data=data):

• DMOD/python/services/dataservice/dmod/dataservice/initial_data_adder_impl.py

  Line 231 in e5aa5d5

  if not self._dataset_manager.add_data(dataset_name=self._dataset_name, dest=self._REAL_CONFIG_FILE_NAME,

• DMOD/python/services/dataservice/dmod/dataservice/initial_data_adder_impl.py

  Line 349 in e5aa5d5

  if not self._dataset_manager.add_data(dataset_name=self._dataset_name, dest=real_cfg_file_name,

• DMOD/python/services/dataservice/dmod/dataservice/initial_data_adder_impl.py

  Line 456 in e5aa5d5

  if not self._dataset_manager.add_data(dataset_name=self._dataset_name,

• DMOD/python/services/dataservice/dmod/dataservice/initial_data_adder_impl.py

  Line 563 in e5aa5d5

  if not self._dataset_manager.add_data(dataset_name=self._dataset_name, dest=item, data=data):

• DMOD/python/lib/core/dmod/test/test_dataset_manager.py

  Line 24 in e5aa5d5

  def add_data(self, dataset_name: str, dest: str, data: Optional[bytes] = None, source: Optional[str] = None,

• DMOD/python/services/dataservice/dmod/test/test_data_inquery_util.py

  Line 29 in e5aa5d5

  def add_data(self, dataset_name: str, dest: str, data: Optional[bytes] = None, source: Optional[str] = None,

• DMOD/python/lib/modeldata/dmod/test/it_object_store_dataset_manager.py

  Line 126 in e5aa5d5

  result = self.manager.add_data(dataset_name=dataset_name, dest=dest_object_name, data=file_to_add.read_bytes(),

• DMOD/python/lib/modeldata/dmod/test/it_object_store_dataset_manager.py

  Line 126 in e5aa5d5

  result = self.manager.add_data(dataset_name=dataset_name, dest=dest_object_name, data=file_to_add.read_bytes(),

[robertbartel]

When there is a change to the data a dataset contains - add or delete - then the dataset's true domain has changed. We don't have everything we need yet to automatically detect this for any and every supported dataset, so we required the changes to the domain be sent along with any changes to the data.

So we can't simply take that out without introducing another breakage.

[aaraney]

So we can't simply take that out without introducing another breakage.

What about constructing a Generic data format / domain if a DataDomain is not provided?

[robertbartel]

That depends on what you mean. The dataset would get a valid data domain that way. But it wouldn't reflect the actual data. So the data service wouldn't be able to find a dataset and use it to fulfill requirements that the dataset could actually cover.

[aaraney]

The dataset would get a valid data domain that way. But it wouldn't reflect the actual data. So the data service wouldn't be able to find a dataset and use it to fulfill requirements that the dataset could actually cover.

Exactly! So, should domain metadata information be opt-in if you are using the CLI tools? Part of me says yes, but I am persuadable.

Aside, I imagine we would do something similar when we have detection capabilities developed. For example, a user uploads a file, we try to detect the domain of that data item, and we cannot detect the domain for the uploaded data item. I don't think we should fail the upload (the file could be quite large) if the upload didnt fail. The metadata detection is what failed, so in that case I think there is a strong argument for using a Generic DataDomain as a placeholder.

[robertbartel]

So, should domain metadata information be opt-in if you are using the CLI tools?

No, not at the moment at least. A proper domain definition for a dataset is necessary, regardless of the source of creation/upload, except perhaps for special, ephemeral situations. Without an accurate (and at least reasonably precise) domain, a dataset isn't really useful. And right now the only place detectors are used is on the client side.

The idea was to provide the available tools to help detect domains in the client, but also accept explicitly provided domains for cases where automated detection wasn't working yet, and then have the server-side API require the domain changes be given along with any data changes. Eventually, when we have working detectors for all formats, we can think about redesign this, but that's the intent right now.

I don't think we should fail the upload (the file could be quite large) if the upload didnt fail.

Again, I don't think that works. It prevents wasting the upload, but it also "disables" the dataset. I.e., if adding the last forcing CSV file fails, instead of a forcing dataset that covers all but one of the intended catchments, you've got something just taking up space until it is somehow fixed (and there is no mechanism for doing that right now).

Also, if we are expecting the updates to the domain to come from the client, I think we can have the failure before an upload, and thus avoid doing it at all.

[robertbartel]

One other wrinkle is that we not only need to detect/acquire an item's domain somewhere when adding a data item, but also ensure that the item's domain can be merged with the dataset's domain. If it can't be merged, once again, I don't think we want to disable a previously working dataset.

[aaraney]

Without an accurate (and at least reasonably precise) domain, a dataset isn't really useful.

I agree in the case of forcing, hydrofabric, and partition datasets, but IMO all other dataset variants don't really benefit from this metadata -- yet.

I don't think its necessarily a bad thing, at this phase in the project, that in some areas our dataset metadata is not "complete" (for lack of a better term). My main concern really comes down to timing and unknown unknowns. To me, this is another case of being the most downstream project with all upstream projects more or less being in alpha or beta. We have little control over upstream change to data formats (e.g. x BMI module init config) and that is okay. We are going to have to just deal with that for the moment. I don't think we should sink a ton of time into worrying about things that we know are not nearing their final form yet. IMO, I think the good citizen approach of adding metadata to a dataset at creation or after the data has been added is the best middle ground for where we are in relation to other projects that we depend on. Forcing, hydrofabric, and partition datasets can reap the benefits from this approach while other dataset variants don't have the same requirements.

In the future, I completely agree, updating a datasets data domain information is a requirement when modifying a dataset. I just don't think we are there yet and that is not a fault of our own or the upstream projects. IMO there are just too many unknown unknowns and axes of change that we cannot control.

[aaraney]

Also, if we are expecting the updates to the domain to come from the client, I think we can have the failure before an upload, and thus avoid doing it at all.

Yeah, I think that is fair if we are talking strictly about the CLI stuff. Would you expect that a service like the evaluation service would also do similar checks prior to uploading data?

[aaraney]

One other wrinkle is that we not only need to detect/acquire an item's domain somewhere when adding a data item, but also ensure that the item's domain can be merged with the dataset's domain. If it can't be merged, once again, I don't think we want to disable a previously working dataset.

In principle, I agree. I do think we need a way to do this forcefully though. So, a way to add data to a dataset without modifying a dataset's domain or modifying it such a way to denote that changes were forcefully made.

[robertbartel]

Yeah, I think that is fair if we are talking strictly about the CLI stuff. Would you expect that a service like the evaluation service would also do similar checks prior to uploading data?

Hmm ... not sure. Perhaps not. Though maybe so: are we using classes from dmod.client in the evaluation service for connecting to the data service (I think we want to)?

Regardless, I still think that a failed attempt to add new data items to a dataset should, in general, not reduce the usability of the dataset from what it was before the "add" attempt. If there are cases when that is undesirable - it initially seems unnecessary in your example, but not problematic - then we probably need to further change the API.

In principle, I agree. I do think we need a way to do this forcefully though. So, a way to add data to a dataset without modifying a dataset's domain or modifying it such a way to denote that changes were forcefully made.

I'm curious whether you mean "without immediately" here - i.e., the domain update will come, just separately - or completely without any domain update. Setting aside the "without immediately" case, while it should be safe for a dataset to contain data beyond what is described by its domain, is there a reason why we'd ever want this?

[christophertubbs]

Though maybe so: are we using classes from dmod.client in the evaluation service for connecting to the data service (I think we want to)?

Nope - the evaluation service isn't connected to the rest of the dmod services because it's not clear how to integrate with or even reliably run the data service. A dmod.client may be used when everything is ready, but REST and file operations are currently what's used to load data in the eval service.

[aaraney]

I agree that when we add data to a dataset we need to update the dataset's domain. At this point in time without dataset detectors implemented for all major dataset types we support this is tedious and just feels like another step. In the future, when this is more automated this will be extremely useful and a must have. Just in the present when we are trying to integrate with other parts of the system and dataset formats are continually evolving (e.g. changing / new bmi init config formats or hydrofabric formats / data model) I would rather slightly neglect some of the dataset intelligence features of DMOD (for now) and keep it dead simple and focus on getting larger components connected (e.g. eval capabilities, GUI) and circle back to this when we have more confidence in running models and conducting evaluations.