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.

Sign up for GitHub

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

Jump to bottom

Remove fix #224

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Remove fix #224

merged 1 commit into from
Jan 27, 2021

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Sep 29, 2020 •
edited
Loading

WIP
Aiming to remove a workaround introduced in #208, supposedly now fixed in python-eccodes.

So far, have re-checked that the original problem is replicable, having forced the old python-eccodes version, and removed the fix. This triggered a segfault in the tests.integration.round_trip.test_grid_definition_section.TestGDT5. It should also hit this in tests/unit/save_rules/test_data_section.TestNonDoubleData (but that is not run before the crash happens).
Then, removed the version pin, now testing with 2.18.0 and python-eccodes 2020.06.0.
This should have been ok, but at present the original crash is still occurring .

Current Status : skipped out the GDT5 test, temporarily, to enable the intended test tests/unit/save_rules/test_data_section.TestNonDoubleData, which targets/demonstrates the problem more clearly.

@pp-mo pp-mo added this to the v0.16.0 milestone Sep 29, 2020
@pp-mo
Copy link
Member Author

pp-mo commented Sep 29, 2020

Ok. I think that shows the same bug, according to discussion in #208

@pp-mo
Copy link
Member Author

pp-mo commented Sep 29, 2020 •
edited
Loading

Oh dear.
It looks like removal of this workaround will have to wait.
I don't understand this, as there have been several python-eccodes since this was supposedly fixed.
😩

trexfeathers
trexfeathers reviewed Sep 29, 2020
View reviewed changes
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.

😥

@lbdreyer
Copy link
Member

It has been fixed in pytho-eccodes but not been released on conda yet, only pip.

@pp-mo
Copy link
Member Author

pp-mo commented Sep 29, 2020 •
edited
Loading

not been released on conda yet, only pip

Well I'm a bit confused by that. The releases available via conda seem to be long after the merge of the referenced issue ?
Anyway, I raised ecmwf/eccodes-python#34. Maybe will get a clarifying response

Meantime, I'm removing this from the 0.16 effort.

@pp-mo pp-mo removed this from the v0.16.0 milestone Sep 29, 2020
@pp-mo pp-mo added Status: Blocked T-Shirt: Small Type: Technical Debt known issues left over from incomplete work labels Sep 29, 2020
@lbdreyer
Copy link
Member

The releases available via conda seem to be long after the merge of the referenced issue ?

The fix (ecmwf/eccodes-python#31) went in at the end of July. The last conda release of python eccodes was at the end of June, THe last pip release was in August. https://github.com/ecmwf/eccodes-python/releases
So it's not available on conda.
I asked when it will be available: ecmwf/eccodes-python#33

@shahramn
Copy link

The segmentation fault issue with float32 is fixed in the next release of eccodes-python (v1.0.0) which will be accompanied by ecCodes v2.19.0
Hopefully in October

@pp-mo pp-mo mentioned this pull request Jan 19, 2021
Merged
@pp-mo pp-mo changed the base branch from master to v0.16.x January 27, 2021 14:58
@pp-mo
Copy link
Member Author

pp-mo commented Jan 27, 2021

Re-fixed and re-targetted at v0.16.x

It's now really simple, so let's hope it checks out..

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 92.821% when pulling 113dc92 on pp-mo:remove_fix into e0ade8d on SciTools:v0.16.x.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 92.821% when pulling 113dc92 on pp-mo:remove_fix into e0ade8d on SciTools:v0.16.x.

@lbdreyer
Copy link
Member

Thanks @pp-mo !

@lbdreyer lbdreyer merged commit 93fc344 into SciTools:v0.16.x Jan 27, 2021
jamesp added a commit to jamesp/iris-grib that referenced this pull request Feb 23, 2021
@jamesp
Merge remote-tracking branch 'upstream/v0.16.x' into cirrus-16
d1b192e 
* upstream/v0.16.x:
  Docstest 0v16 (SciTools#244)
  Remove eccodes bug workaround added in SciTools#208. (SciTools#224)
  Update requirements to pick up Iris 3. (SciTools#243)
  Add 'main' conda-forge channel, needed for docs builds. (SciTools#240)
  Fix RC date in release notes (about to cut). (SciTools#235)
  Version 0.16 release candidate (SciTools#232)
  Cosmetic change : rename the travis iris-test-version options (SciTools#234)
  Travis test with  both Iris latest-release and latest-master. (SciTools#231)
lbdreyer pushed a commit that referenced this pull request Feb 24, 2021
@pp-mo @lbdreyer
Remove eccodes bug workaround added in #208. (#224)
7b53510
bjlittle pushed a commit that referenced this pull request Mar 2, 2021
@jamesp @pp-mo
Cirrus CI (#250)
1a8be59 
* Travis test with  both Iris latest-release and latest-master. (#231)

* Travis test with  both Iris latest-release and latest-master.

* Modify test CMLs for latest Iris (Iris3.0 changes).

* Grib1 load fixes.

* Fix loading since units=None default for Iris3 coords

* Modify test to work with latest Iris (Iris3.0 changes).

* Test against latest Iris only.

* Review changes.

* Cosmetic change : rename the travis iris-test-version options (#234)

* Rename Iris test-version options, and enable all to check action.

* Review changes.

* Version 0.16 release candidate (#232)

* Require Iris >=3 (just for the gdt90 changes).

* Whatsnew entry for requiring Iris 3.

* Set version string for release candidate.

* Test against Iris 3.0.0rc0 from conda-forge/rc_iris.

* Fix RC date in release notes (about to cut). (#235)

* Add 'main' conda-forge channel, needed for docs builds. (#240)

* Add 'main' conda-forge channel, needed for docs builds.

* Use fixed spherical-earth-radius in GRIB1, ignoring change in gribapi default.

* Codestyle fix.

* Update requirements to pick up Iris 3. (#243)

* Remove eccodes bug workaround added in #208. (#224)

* Docstest 0v16 (#244)

* Document PR#240 in release notes.

* Fix version string and release-notes date.

* Added getting started .cirrus.yml

* Update cirrus to use miniconda image

* Update .cirrus.yml

* Update .cirrus.yml

Copied across cirrus file from iris-ugrid

* Added nox testing

Borrowing from iris and iris-ugrid, added nox testing and a test runner.

Tests currently fail on my local machine.

* Path fixes in cirrus.yml

* eccodes test added to noxfile

* Added config and coverage

* Trying to set SITE_CFG

* syntax error

* taking the IRIS_DIR from Travis CI config

* Add allow_failures to the linux task for now

* Dodgy r key...

* Update .cirrus.yml

* Allow failures in linting

* Force return error 0  for now

* Added eccodes test

* moved matrix

* Yaml syntax error & * wrong way around

* Typo

* Update iris version dependency

* Date fix

This resolves the same bash issue as SciTools/iris#4019

* Configure Iris in nox

* Updated CI config

* correct site-packages path

* Fix to iris test data path

* force cache update

* refactored writing iris config in noxfile

* Fix pep8 and license test fails

* Support for testing against packaged iris and building from source

* Adding yaml to list of cirrus container requirements

* invalidate cirrus cache

* really invalidate cirrus cache...

* yaml -> pyyaml

* missing iris_dir reference

* call write_iris_config

* docstrings and matrix testing

* git syntax error

* eccodes selfcheck in basic tests

* Removed py3.8 testing for now

* Fixed regression in setup.py test

* Removed unused imports

* Disabled lint checking

* Removed nox from test dependencies for python3.6 and python3.8

* Removed python3.8 from noxfile

* Removed nox from test requirements

Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk>
@pp-mo pp-mo deleted the remove_fix branch March 3, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trexfeathers trexfeathers trexfeathers left review comments

@lbdreyer lbdreyer Awaiting requested review from lbdreyer

Assignees
No one assigned
Labels
Status: Blocked T-Shirt: Small Type: Technical Debt known issues left over from incomplete work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pp-mo @lbdreyer @shahramn @coveralls @trexfeathers