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

Better handling and documentation of dependencies #1589

Merged
merged 28 commits into from
Dec 1, 2021
Merged

Conversation

tjtg
Copy link
Contributor

@tjtg tjtg commented Oct 19, 2021

This PR is intended to improve handling of dependencies and conda environments used for testing.

  • Pin package versions in existing example environments so that these environments are more explicit.
  • Rename existing example environments to avoid naming which is specific to the package versions used.
  • Add two more environments:
    • latest - full set of packages, as recent versions and minimal pinning as possible
    • conda-forge - a minimal environment intended to match required dependencies listed on the conda-forge feedstock
  • Remove dependencies from setup.cfg.
    • The expectation is that direct usage of setup.cfg (eg. pip install -e .) is by developers who will provide their own environment such as from conda.
    • Other non-developer users are likely to install from conda-forge (eg. conda install -c conda-forge improver) and will pick up dependencies from that package.
    • Installing major IMPROVER dependencies such as Iris and Cartopy from PyPI is complex due to dependency on non-Python packages such as Proj.
  • Fix the exclusion of improver_tests - previously the top level package was being excluded, but subpackages were being included. This resulted in all the test files being installed, but in a non-working state.
    • It's desirable to add running of the unit tests as part of the conda-forge package build process, but this can be done by copying in the test files as part of the conda recipe.
  • Add a documentation page to explain the use of dependencies and which CLIs are affected by optional dependencies not being available.
  • Remove use of python-dateutil. This was only used in one location in the code, which was easy to replace with Python standard library datetime functionality.
  • Remove a unit test assertion which depended on the timezone database, causing different results depending on the timezone database (via pytz) version in use.

Testing:

  • Ran tests and they passed OK
  • Added new tests for the new feature(s)

@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #1589 (c2c7591) into master (c35f243) will decrease coverage by 0.57%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1589      +/-   ##
==========================================
- Coverage   98.06%   97.49%   -0.58%     
==========================================
  Files         110      111       +1     
  Lines        9984    10136     +152     
==========================================
+ Hits         9791     9882      +91     
- Misses        193      254      +61     
Impacted Files Coverage Δ
improver/metadata/amend.py 100.00% <100.00%> (ø)
...ometric_calculations/psychrometric_calculations.py 99.21% <100.00%> (-0.01%) ⬇️
improver/ensemble_copula_coupling/utilities.py 96.55% <0.00%> (-3.45%) ⬇️
improver/__init__.py 95.65% <0.00%> (-0.35%) ⬇️
...semble_copula_coupling/ensemble_copula_coupling.py 99.04% <0.00%> (-0.01%) ⬇️
improver/regrid/grid.py 100.00% <0.00%> (ø)
improver/calibration/dataframe_utilities.py 100.00% <0.00%> (ø)
...prover/ensemble_copula_coupling/numba_utilities.py 14.28% <0.00%> (ø)
improver/regrid/landsea.py 99.21% <0.00%> (+0.09%) ⬆️
... and 2 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 c35f243...c2c7591. Read the comment docs.

@tjtg tjtg added BoM review required PRs opened by non-BoM developers that require a BoM review MO review required PRs opened by non-Met Office developers that require a Met Office review labels Oct 19, 2021
dmentipl
dmentipl previously approved these changes Oct 20, 2021
Copy link
Contributor

@dmentipl dmentipl left a comment

Choose a reason for hiding this comment

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

Thanks @tjtg. This provides a substantial improvement on the state of environments, dependencies, and documentation of both.

One small comment: the "conda-forge" and "latest" environment files have the same "name", i.e. "improver_latest". Do you think this will cause confusion? Perhaps the conda-forge env could be named something like "improver_conda_forge" or something similar?

@tjtg
Copy link
Contributor Author

tjtg commented Oct 20, 2021

the "conda-forge" and "latest" environment files have the same "name", i.e. "improver_latest". Do you think this will cause confusion? Perhaps the conda-forge env could be named something like "improver_conda_forge" or something similar?

That's a mistake from copy-pasting. Yes, it should be named something else and I'll add a commit to do that.

dmentipl
dmentipl previously approved these changes Oct 20, 2021
Copy link
Contributor

@dmentipl dmentipl left a comment

Choose a reason for hiding this comment

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

All good now. Thanks @tjtg.

Copy link
Contributor

@MoseleyS MoseleyS left a comment

Choose a reason for hiding this comment

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

Please can we drop the requirement to pass all tests with "latest"; we don't want to support that as I would expect it to fail nearly all the time. I'm not sure of the value in supporting the "conda-forge" environment either.

I've spoken to @benfitzpatrick about this and he would like to discuss this with @tjtg next week.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@MoseleyS MoseleyS assigned tjtg and unassigned MoseleyS Nov 4, 2021
@MoseleyS
Copy link
Contributor

Thanks @tjtg for sending me some more information about this PR. Here's the bits that I think ought to be included here, so we remember why we've set these up in this way.

  • All existing tests must pass with environment_a, although this doesn't have to be the case in the future (I'm thinking of tasks requiring Numba which I'm expecting to go into b first, and a later).
  • A subset of tests must also pass with environment_b. Exclusions are listed at the top of the YAML file.
  • The conda-forge environment only needs some basic things to pass. It's purpose is to allow anyone to install IMPROVER and its dependencies using Conda.
  • The latest environment will run periodically (daily?), but not as part of each PR. This will allow us to detect when external changes to things IMPROVER is dependent on have had an adverse effect, allowing us to decide what to do about each impact.
  • Timezone tests will remain, but for a small area centred over Europe where breaking changes (from political changes) are expected to be infrequent.

MoseleyS
MoseleyS previously approved these changes Nov 12, 2021
.github/workflows/scheduled.yml Outdated Show resolved Hide resolved
MoseleyS
MoseleyS previously approved these changes Nov 23, 2021
@tjtg
Copy link
Contributor Author

tjtg commented Nov 24, 2021

I think all the remaining items have been addressed now - ready for @MoseleyS and @cpelley to re-review.

196c83e sets up daily running of the latest environment. During this setup, I noticed that:

  • The scheduled action will only run on the default branch (master for this repository).
  • The start time of the run can vary significantly from what is specified via the cron syntax. Some more frequent test actions I ran (in a separate repository) varied a lot from what was specified in both start time and spacing between, but hopefully daily is infrequent enough that this one works OK. We'll have to see how this goes in practice after it's merged to master.

I've added on: workflow_dispatch which provides a button in the web interface to manually run the action. This gives us a fallback option in case the scheduling doesn't work as intended.

c2c7591 adds tests for timezone mask generation using a regular lat/lon grid over western Europe. This corresponds to the last item in comment above #1589 (comment).

@MoseleyS
Copy link
Contributor

I'm tentatively approving this PR. I'd like @bayliffe to review the last change (to the timezone unit-tests) as he is the most familiar with them in our team. I'm happy with all the rest.

@bayliffe
Copy link
Contributor

Thanks @tjtg for the changes to the timezone tests. You've added a little more rigour, which is welcome. I have no issue with the changes.

@tjtg tjtg merged commit 91078bb into metoppv:master Dec 1, 2021
tjtg added a commit to tjtg/improver-feedstock that referenced this pull request Dec 3, 2021
tjtg added a commit to tjtg/improver-feedstock that referenced this pull request Dec 3, 2021
tjtg added a commit to conda-forge/improver-feedstock that referenced this pull request Dec 3, 2021
MoseleyS added a commit that referenced this pull request Jan 18, 2022
* master:
  Remove __repr__ methods from all neighbourhood plugins (#1648)
  ENH: Avoiding lazy loading in select command calls (#1617)
  MOBT-180: Weather symbol speed up (#1638)
  IM-1621: Make ECC error and warning tests more rigorous (#1641)
  Make flake8 report that it is okay when running improver-tests. (#1645)
  Update checksums after updating the title of files in apply-emos-coefficients/sites. (#1640)
  Fixes bug in spot-extraction for multi-time inputs (#1633)
  Updates checksums for threshold landmask fix (#1636)
  Update interpret-metadata (#1632)
  Weather code tree update (#1635)
  Fix noise in precip accumulation thresholds (#1627)
  Expanding on triangle time blending doc strings. (#1630)
  Better handling and documentation of dependencies (#1589)
  Add tests (#1626)
  Enhancements on new regridding code (#1560)
  Speed up interpolation in ensemble_copula_coupling.ConvertProbabilitiesToPercentiles (#1578)
  Speed up interpolation in ensemble_copula_coupling.ResamplePercentiles (#1548)
  Spot-extraction additional coordinates ordering fix (#1610)
MoseleyS added a commit that referenced this pull request Jan 18, 2022
* upstream/master:
  Remove __repr__ methods from all neighbourhood plugins (#1648)
  ENH: Avoiding lazy loading in select command calls (#1617)
  MOBT-180: Weather symbol speed up (#1638)
  IM-1621: Make ECC error and warning tests more rigorous (#1641)
  Make flake8 report that it is okay when running improver-tests. (#1645)
  Update checksums after updating the title of files in apply-emos-coefficients/sites. (#1640)
  Fixes bug in spot-extraction for multi-time inputs (#1633)
  Updates checksums for threshold landmask fix (#1636)
  Update interpret-metadata (#1632)
  Weather code tree update (#1635)
  Fix noise in precip accumulation thresholds (#1627)
  Expanding on triangle time blending doc strings. (#1630)
  Better handling and documentation of dependencies (#1589)
  Add tests (#1626)
  Enhancements on new regridding code (#1560)
  Speed up interpolation in ensemble_copula_coupling.ConvertProbabilitiesToPercentiles (#1578)
  Speed up interpolation in ensemble_copula_coupling.ResamplePercentiles (#1548)
  Spot-extraction additional coordinates ordering fix (#1610)
@tjtg tjtg deleted the depupdate branch May 2, 2022 12:25
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
* Rename and add environments

* Thin out requirements in setup.cfg, add note

* Remove improver_tests tests from built package

The previous exclude was not effective - it excluded the top level
directory, but not the subdirectories containing all the test source
code files.

* Skip checksum sorted test if file not available

* Pytest skips for stratify

* Move stratify import inside function

* Remove duplicated statsmodels

* Better explanation for latest environment

* Add sphinx typehints to conda-forge tests section

* Run security checks on all environments

* Separate coverage and no-coverage environments

* Add detailed pinning to A/B environments

* Pin numpy/cartopy in latest environment

Unit test failures occur with older versions

* Start documentation

* Remove dateutil dependency

* More documentation

* Remove timezone database dependence in unit test

* Fix duplicated name in conda-forge environment

* Add note that not all CLIs are available with environment_b

* Remove leftover expected data from test_process

* Fix flake8

* Remove not-really-working install_requires section

* Clarify conda-forge environment comment

* Move latest environment tests to schedule

* Fix actions YAML

* Reschedule

* Fix scheduling, add manual running, metoppv-only

* Add timezone mask test on europe lat/lon domain
MoseleyS added a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
* upstream/master:
  Remove __repr__ methods from all neighbourhood plugins (metoppv#1648)
  ENH: Avoiding lazy loading in select command calls (metoppv#1617)
  MOBT-180: Weather symbol speed up (metoppv#1638)
  IM-1621: Make ECC error and warning tests more rigorous (metoppv#1641)
  Make flake8 report that it is okay when running improver-tests. (metoppv#1645)
  Update checksums after updating the title of files in apply-emos-coefficients/sites. (metoppv#1640)
  Fixes bug in spot-extraction for multi-time inputs (metoppv#1633)
  Updates checksums for threshold landmask fix (metoppv#1636)
  Update interpret-metadata (metoppv#1632)
  Weather code tree update (metoppv#1635)
  Fix noise in precip accumulation thresholds (metoppv#1627)
  Expanding on triangle time blending doc strings. (metoppv#1630)
  Better handling and documentation of dependencies (metoppv#1589)
  Add tests (metoppv#1626)
  Enhancements on new regridding code (metoppv#1560)
  Speed up interpolation in ensemble_copula_coupling.ConvertProbabilitiesToPercentiles (metoppv#1578)
  Speed up interpolation in ensemble_copula_coupling.ResamplePercentiles (metoppv#1548)
  Spot-extraction additional coordinates ordering fix (metoppv#1610)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BoM review required PRs opened by non-BoM developers that require a BoM review MO review required PRs opened by non-Met Office developers that require a Met Office review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants