Skip to content

Data Service Rest Refactor -- Library Code#327

Closed
aaraney wants to merge 30 commits intoNOAA-OWP:masterfrom
aaraney:refactor-data-service-2
Closed

Data Service Rest Refactor -- Library Code#327
aaraney wants to merge 30 commits intoNOAA-OWP:masterfrom
aaraney:refactor-data-service-2

Conversation

@aaraney
Copy link
Copy Markdown
Member

@aaraney aaraney commented Apr 24, 2023

The second phase of refactoring the data service. This phase focuses on rewriting library code that supports reading and writing to and from the object store (related #323). Much of this work will eventually be migrated into dmod.metadata, but for now, it will live in the service as it is the sole user.

Additions

  • DatasetClient and DatasetQueryClient. These classes separate the concerns of reading and writing to a data store (e.g. minio) and functionally replace ObjectStoreDatasetManager.
  • Result type that conceptually functions like a enum of either Ok(T) or Err(E). This forces users to explicitly handle errors and enables treating exceptions types as values. This addition was heavily inspired by Rust's Result type.
  • Picked up exceptiongroup. "This is a backport of the BaseExceptionGroup and ExceptionGroup classes from Python 3.11." source.

Testing

  1. Unit and integration tests for DatasetClient and DatasetQueryClient's are added.

Todos

  • Add integration tests (this is mostly done)
  • Add doc strings
  • Add unit tests

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Comment thread python/services/dataservice/dmod/dataservice/dataset.py
@aaraney aaraney marked this pull request as ready for review May 2, 2023 17:18
@aaraney aaraney requested a review from robertbartel May 2, 2023 17:18
@robertbartel robertbartel added enhancement New feature or request maas MaaS Workstream labels Jun 9, 2023
@robertbartel
Copy link
Copy Markdown
Contributor

@aaraney, I lost track of this one previously. It needs to be rebased after the recent merges. I'll probably start glancing at it, but I'll wait to perform the review in earnest until that's done.

@aaraney aaraney force-pushed the refactor-data-service-2 branch from 35306c6 to b1cc93d Compare June 9, 2023 17:02
@aaraney
Copy link
Copy Markdown
Member Author

aaraney commented Jun 9, 2023

@robertbartel, ive rebased and force pushed the changes.

@robertbartel
Copy link
Copy Markdown
Contributor

robertbartel commented Jun 9, 2023

@aaraney, the first thing I noticed starting my review was the duplication of several Dataset related things, including the Dataset class. I vaguely recall a conversation - perhaps not about this but something similar - where the plan was to duplicate several types temporarily while we waited on the Pydantic refactoring work, in order to give Pydantic functionality to the temporary, copied types.

With the Pydantic work now finished, I want to confirm before I get too deep into the review process: do we still need to duplicate things like this?

@aaraney
Copy link
Copy Markdown
Member Author

aaraney commented Jun 9, 2023

@robertbartel, your memory is correct. The majority if not all of the duplication can be removed now that #331 has been merged. I am going to move this to draft status for now until I address the unnecessary duplication.

@aaraney aaraney marked this pull request as draft June 9, 2023 18:11
@aaraney aaraney closed this May 15, 2024
@aaraney
Copy link
Copy Markdown
Member Author

aaraney commented May 15, 2024

This is superseded by the work introduced in #575. Some of the ideas and data transfer objects may be applicable to future work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request maas MaaS Workstream

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants