Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exception in datacube dataset update for numeric keys #983

Merged
merged 2 commits into from Jul 6, 2020

Conversation

jeremyh
Copy link
Contributor

@jeremyh jeremyh commented Jul 6, 2020

The dataset update command threw an exception when one of the unsafe changes was a numeric key:

File "/g/data/v10/public/modules/dea/20200526/lib/python3.6/site-packages/datacube/index/_datasets.py", line 284, in <genexpr>
    full_message = "Unsafe changes at " + ", ".join(".".join(offset) for offset, _, _ in unsafe_changes)
TypeError: sequence item 2: expected str instance, int found

This fixes that, and updates the product update, metadata update and dataset update commands to display document changes more consistently with each-other.

Other minor issues fixed:

  • It now tells you which document the error occurred in (since we often have multiple documents in one yaml)
  • The unsafe value changes are now shown as warnings so that they display by default without needing to rerun the command with higher verbosity.

Issues not yet fixed:

It's a pain to make changes to a multi-document yaml. I wish I could tell it "update the ODC to match my whole yaml", without having to pick apart which inner-documents need updating and which need adding. Are we sure add and update need to be mutually exclusive?

(An example of multi-doc yamls are DEA's metadata-types and products)

- Datasets threw exceptions when the offset was not a string.
- Display unsafe changes at warning level, so they're displayed by default.
Multiple documents can be a single yaml, so the tools should tell which entity contains the error.
@jeremyh jeremyh changed the title Fix exception in datacube metadata update for numeric keys Fix exception in datacube dataset update for numeric keys Jul 6, 2020
@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #983 into develop will increase coverage by 0.02%.
The diff coverage is 99.43%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #983      +/-   ##
===========================================
+ Coverage    92.95%   92.98%   +0.02%     
===========================================
  Files           97       97              
  Lines         9649     9703      +54     
===========================================
+ Hits          8969     9022      +53     
- Misses         680      681       +1     
Impacted Files Coverage Δ
datacube/index/eo3.py 100.00% <ø> (ø)
datacube/ui/click.py 84.31% <0.00%> (-0.11%) ⬇️
datacube/utils/geometry/__init__.py 100.00% <ø> (ø)
datacube/api/core.py 98.37% <100.00%> (+<0.01%) ⬆️
datacube/api/grid_workflow.py 92.91% <100.00%> (-0.22%) ⬇️
datacube/drivers/netcdf/_write.py 100.00% <100.00%> (ø)
datacube/drivers/netcdf/driver.py 100.00% <100.00%> (ø)
datacube/drivers/postgres/_core.py 95.60% <100.00%> (ø)
datacube/index/_datasets.py 94.60% <100.00%> (+0.25%) ⬆️
datacube/index/_metadata_types.py 94.79% <100.00%> (-0.06%) ⬇️
... and 16 more

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 dae57fb...8691ebc. Read the comment docs.

@omad omad self-requested a review July 6, 2020 02:13
Copy link
Member

@omad omad left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough changes Jeremy, looks good to me.

Could you please make an issue to discuss the semantics of add vs update.

My thoughts in brief (I'll copy to an issue when it exists):

  • Multi-document YAMLs add significant complexity to quite a few of our operations
  • They are useful for storing related resources together,
  • When editing it's not obvious if or how many extra documents are in the same file.
  • Kubernetes also uses YAML for storing configuration, and allows multiple completely different records in the same file
  • It's CLI apply command takes your suggested approach of "make the database match what's in my file", regardless of whether it's adding/update, while also checking that it's a safe change.
  • If we're going to keep supporting multiple document YAMLs, I think we should support this operation. But I'm not sure what name to use, and I'm not sure if we should keep add and update.
Crazy Damien idea that should not be seen

Let's implement an ODC Kubernetes Operator, which you interact with to update an ODC Database via Kubernetes tools

@omad omad merged commit 8152f8c into opendatacube:develop Jul 6, 2020
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.

None yet

2 participants