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

Use "xcube level" tool with S3 #649

Merged
merged 12 commits into from
Mar 16, 2022
Merged

Use "xcube level" tool with S3 #649

merged 12 commits into from
Mar 16, 2022

Conversation

forman
Copy link
Member

@forman forman commented Mar 16, 2022

This PR closes #617 and addresses #516:

  • The xcube level CLI tool has been rewritten from scratch to make use
    of xcube filesystem data stores. (Allow creating pyramids on S3 #617)

  • Deprecated numerous classes and functions around multi-level datasets.
    The non-deprecated functions and classes of xcube.core.mldataset should
    be used instead along with the xcube filesystem data stores for
    multi-level dataset i/o. (Unify xcube dataset i/o #516)

    • Deprecated all functions of the xcube.core.level module
      • compute_levels()
      • read_levels()
      • write_levels()
    • Deprecated numerous classes and functions of the xcube.core.mldataset
      module
      • FileStorageMultiLevelDataset
      • ObjectStorageMultiLevelDataset
      • open_ml_dataset()
      • open_ml_dataset_from_object_storage()
      • open_ml_dataset_from_local_fs()
      • write_levels()
  • Support for multi-level datasets has been improved:

    • Introduced new parameters for writing multi-level datasets with the "file", "s3", and "memory" data stores. They are
      • ...
      • num_levels: If given, restricts the number of resolution levels
        to the given value. Must be a positive integer to be effective.

Closes #617.

Checklist:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in CHANGES.md
  • AppVeyor CI passes
  • Test coverage remains or increases (target 100%)

FYI @AliceBalfanz @pont-us

@forman forman requested a review from TonioF March 16, 2022 08:59
@forman forman self-assigned this Mar 16, 2022
@forman forman changed the title Forman 617 levels on s3 v2 Use "xcube level" tool with S3 Mar 16, 2022
@forman forman marked this pull request as ready for review March 16, 2022 10:55
@codecov-commenter
Copy link

Codecov Report

Merging #649 (7923c82) into master (a19aa3c) will decrease coverage by 0.00%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
- Coverage   91.96%   91.95%   -0.01%     
==========================================
  Files         298      298              
  Lines       27335    27400      +65     
==========================================
+ Hits        25138    25197      +59     
- Misses       2197     2203       +6     
Impacted Files Coverage Δ
xcube/cli/level.py 91.83% <86.20%> (-8.17%) ⬇️
xcube/core/mldataset.py 79.32% <94.73%> (+1.03%) ⬆️
test/cli/helpers.py 100.00% <100.00%> (ø)
test/cli/test_level.py 100.00% <100.00%> (ø)
test/core/test_mldataset.py 99.39% <100.00%> (+0.03%) ⬆️
xcube/core/level.py 84.04% <100.00%> (-0.57%) ⬇️
xcube/core/store/__init__.py 100.00% <100.00%> (ø)
xcube/core/store/fs/impl/mldataset.py 89.74% <100.00%> (+0.77%) ⬆️

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 a19aa3c...7923c82. Read the comment docs.

Copy link
Contributor

@TonioF TonioF left a comment

Choose a reason for hiding this comment

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

Minor suggestions for improvement, but overall it looks good!



@deprecated(version=_DEPRECATED_SINCE, reason=_DEPRECATED_WRITE)
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason does not match.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, becasue it was used when writing only.

@@ -45,6 +46,12 @@
from xcube.util.perf import measure_time
from xcube.util.tilegrid import TileGrid

_DEPRECATED_VERSION = '0.10.2'
Copy link
Contributor

Choose a reason for hiding this comment

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

As there is the theoretical possibility that we might have different deprecation versions in the same module, I'd propose to consider this in the name. Or even put this in a dedicated module somewhere, so we really only have to declare it once.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a private constant and it stands for exactly one constant value used here. If we need more versions one day, we can easily add more constants.

Co-authored-by: Tonio Fincke <tonio.fincke@brockmann-consult.de>
@forman forman merged commit 69aa4db into master Mar 16, 2022
@forman forman deleted the forman-617-levels_on_s3_v2 branch March 31, 2022 07:03
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.

Allow creating pyramids on S3
3 participants