Skip to content

Refactor StateQuery to provide download methods for child handlers#209

Merged
mhidas merged 20 commits into
masterfrom
broker-refactor
Jan 18, 2021
Merged

Refactor StateQuery to provide download methods for child handlers#209
mhidas merged 20 commits into
masterfrom
broker-refactor

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented May 11, 2020

Following on from #208

This is how the above PR should have been implemented. The StateQuery instance represents the high level "public" interface to any existing state. This provides clarity to child handlers by providing a single "API".

  • child handlers can access the resources they need in a simple and consistent way
  • breaking changes to aodncore are less likely, since we don't have to worry about a bunch of different edge cases of handlers importing low level objects, and we can maintain and extend this API as appropriate

In addition, to reduce boilerplate in aodndata when adding custom files after input file resolution (e.g. products), PipelineFile's check_type and publish_type attributes may be set during initialisation time and a helper method was added to HandlerBase in order to automatically assign the file update callback attribute.

These are real world examples of recurring patterns in aodndata (which are currently "correct" given the current library, but should be improved):

Before

  1. files must explicitly add callback or useful logging information about state changes is lost
  2. setter(s) for check_type/publish_type must be called separately after initialisation
            product_file = PipelineFile(product_url, file_update_callback=self._file_update_callback)
            product_file.publish_type = PipelineFilePublishType.HARVEST_UPLOAD
            self.file_collection.add(product_file)

After

  1. file is created in one statement
  2. child handlers don't need to deal with the callback directly
product_file = PipelineFile(product_path, 
                            publish_type=PipelineFilePublishType.HARVEST_UPLOAD)
self.add_to_collection(product_file)

# OR add the path directly to have the object constructed for you
self.add_to_collection(product_path, publish_type=PipelineFilePublishType.HARVEST_UPLOAD)

Before

  1. handlers wanting to download had to access private attributes
self.input_file_collection.download(self._upload_store_runner.broker, self.temp_dir)
# OR
self.state_query._storage_broker.download(
                RemotePipelineFileCollection(old_metadata_file), (self.temp_dir)
            )

After

  1. handlers can download using supported public methods on the handler's state_query instance, without having to dig out the private storage broker objects to do it (as it is intentional that child handlers don't use these directly)
collection = self.state_query.query_wfs_urls('my_layer')
self.state_query.download(collection)

@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2020

Codecov Report

Merging #209 (042c478) into master (b547fed) will increase coverage by 0.12%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   91.69%   91.81%   +0.12%     
==========================================
  Files          36       36              
  Lines        3647     3664      +17     
  Branches      416      418       +2     
==========================================
+ Hits         3344     3364      +20     
+ Misses        251      249       -2     
+ Partials       52       51       -1     
Impacted Files Coverage Δ
aodncore/util/wfs.py 92.72% <86.66%> (-7.28%) ⬇️
aodncore/pipeline/files.py 98.32% <100.00%> (+0.21%) ⬆️
aodncore/pipeline/handlerbase.py 95.28% <100.00%> (+0.47%) ⬆️
aodncore/pipeline/statequery.py 92.85% <100.00%> (+17.85%) ⬆️
aodncore/pipeline/steps/resolve.py 95.63% <100.00%> (ø)
aodncore/version.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b547fed...042c478. Read the comment docs.

Copy link
Copy Markdown
Contributor

@mhidas mhidas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you're still working on this... I had a quick look and made a few comments. I think this will be helpful, but it will introduce breaking changes, so will required updates in aodndata deployed in parallel.

Comment thread aodncore/pipeline/statequery.py
Comment thread aodncore/pipeline/statequery.py
Comment thread aodncore/pipeline/files.py Outdated
@ghost
Copy link
Copy Markdown
Author

ghost commented May 20, 2020

it will introduce breaking changes

It shouldn't introduce any breaking changes. I've run a quick test of aodndata against this branch and made sure any deprecated methods still work, but show a deprecation warning. From this point of, I don't want to break the API and require deployment shenanigans to avoid breakages. That's why I'm keen with this to formalise the "public API" of aodncore, and then audit aodndata to make sure it isn't going into parts of the library that it shouldn't.

@ghost ghost force-pushed the broker-refactor branch from e4c0002 to 4eed8f4 Compare July 7, 2020 08:17
@ghost ghost force-pushed the broker-refactor branch from 4eed8f4 to e7b786f Compare September 4, 2020 03:38
@ghost ghost marked this pull request as ready for review September 4, 2020 03:43
@ghost ghost force-pushed the broker-refactor branch from e7b786f to a13bf9d Compare November 4, 2020 06:23
@ghost ghost force-pushed the broker-refactor branch from a13bf9d to 2489daf Compare November 26, 2020 23:34
@ghost ghost force-pushed the broker-refactor branch from 2489daf to 425af6a Compare January 13, 2021 04:00
Copy link
Copy Markdown
Contributor

@mhidas mhidas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I made a few comments with optional suggestions, but otherwise it's good to go.

Comment thread aodncore/pipeline/handlerbase.py Outdated
Comment thread aodncore/pipeline/statequery.py
Comment thread aodncore/pipeline/statequery.py Outdated
Comment thread aodncore/pipeline/statequery.py Outdated
Comment thread aodncore/pipeline/handlerbase.py
Copy link
Copy Markdown
Contributor

@mhidas mhidas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good! Just added another idea in a separate inline comment (above), but not sure if that's worth looking into now.
Otherwise, just two final comments:

  • Since this introduces new non-breaking API changes, would a minor version bump be warranted?
  • Also, see conflict in test_dummyHandler

@ghost ghost force-pushed the broker-refactor branch from c51850c to 8835d93 Compare January 18, 2021 05:15
@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 18, 2021

@mhidas I've fixed the merge conflict, which was introdiced by #225 being merged in the meantime.

@mhidas mhidas merged commit 8f8ea35 into master Jan 18, 2021
@mhidas mhidas deleted the broker-refactor branch January 18, 2021 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants