Skip to content

Conversation

@ESadek-MO
Copy link
Contributor

@ESadek-MO ESadek-MO commented Mar 6, 2025

Pull Request

Started off as a solution to recent test failures brought on from SciTools/iris#6342, and ended up shuffling the furniture.

This no longer relies on iris, which hopefully means iris-grib tests won't fail whenever something changes in iris. A couple of things to note:

  • Should this still be placed in iris_integration tests? Presumably not.
  • I've left in some commented code, which is bad form, but it's essentially a to-do.
  • The tests failing seem to be due to not running on pytest from what I can see, which begs the question, do we abandn this PR, and return to unit test and just fix the immediate failures?

@ESadek-MO ESadek-MO marked this pull request as draft March 6, 2025 17:12
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.81%. Comparing base (92154ca) to head (7225b6b).
Report is 39 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #620   +/-   ##
=======================================
  Coverage   89.81%   89.81%           
=======================================
  Files           8        8           
  Lines        2475     2475           
  Branches      406      406           
=======================================
  Hits         2223     2223           
  Misses        155      155           
  Partials       97       97           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@trexfeathers trexfeathers 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, some neat use of PyTest best practice.

I know @pp-mo already signed off on the removal of Iris from these tests.

@trexfeathers trexfeathers marked this pull request as ready for review March 12, 2025 11:50
@trexfeathers trexfeathers merged commit 98462bd into SciTools:main Mar 12, 2025
10 checks passed
@pp-mo pp-mo linked an issue Mar 12, 2025 that may be closed by this pull request
@pp-mo
Copy link
Member

pp-mo commented Mar 12, 2025

Just a bit of a remaining worry here, I think.
Isn't the whole point of our interest in pickling that it is essential to the use of dask.distributed ?
In which case, the fact that you can't successfully reconstruct a cube based on a grib load is still a problem I think.
So I don't think we're properly fixed here. TBC #202

@pp-mo pp-mo linked an issue Mar 12, 2025 that may be closed by this pull request
@ESadek-MO
Copy link
Contributor Author

Just a bit of a remaining worry here, I think. Isn't the whole point of our interest in pickling that it is essential to the use of dask.distributed ? In which case, the fact that you can't successfully reconstruct a cube based on a grib load is still a problem I think. So I don't think we're properly fixed here. TBC #202

The load back had been excluded before I changed these tests. All I did is copy the code from Iris and add it to the TODO here, so that it was apparent what code was missing. If it had been fixed, these tests were outdated, and I have not edited the content of the code outside fo the pickle protocol and structure.

@pp-mo
Copy link
Member

pp-mo commented Mar 12, 2025

The load back had been excluded before I changed these tests .... If it had been fixed, these tests were outdated, and I have not edited the content of the code outside of the pickle protocol and structure.

OK very sorry 😢 I had not realised that !!

I had assumed that this got fixed at some point and so that #202 "should have been" closed !!

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.

Latest iris broke pickle testing

4 participants