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

Latest eccodes + workaround seg fault when saving #208

Merged
merged 2 commits into from
Jun 2, 2020

Conversation

lbdreyer
Copy link
Member

@lbdreyer lbdreyer commented May 31, 2020

Previously, we had a found a seg fault was being raised when trying to save out a large array.

This has now been mostly fixed in eccodes, but there is still an outstanding issue.
When trying to save out to GRIB, if the data is not float64, GRIB tries to cast it to float64. This is raising a seg fault!

As a temporary workaround, if we cast the data upfront, is seems to avoid the seg fault.

This PR is made up of 2 commits:

  • the first commit unpins the eccodes requirements such that we get the latest eccodes, and also adds a test that explicitly tests saving int32, int64 and float32 (all of which would need casting to doubles). As you can see from the travis testng, this raises a seg fault
  • the second commit adds the temporary workaround to cast the data to int64 upfront.

Note it turns out the seg fault was also being raised by the TestGDT5 but I thought I test specific to testing the data_section would be more helpful in future, rather than a rountripping test that just so happens to catch the issue.

This is has successfully been tested with the code that demonstrated the original seg fault from the two users who raised this. This has also been successfully tested with the iris-master tests (since there are still a couple grib-other fileformat interoperability tests.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 92.549% when pulling 643487c on lbdreyer:eccodes_bug into 12b989b on SciTools:master.

@lbdreyer
Copy link
Member Author

lbdreyer commented Jun 3, 2020

Thanks for merging @pp-mo !

@GoPavel
Copy link

GoPavel commented Jul 31, 2020

Fortunately, seems Eccodes fixed segfault and will add that in the next release.
ecmwf/eccodes-python#30

pp-mo added a commit to pp-mo/iris-grib that referenced this pull request Jan 27, 2021
jamesp added a commit to jamesp/iris-grib that referenced this pull request Feb 23, 2021
* 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)
bjlittle pushed a commit that referenced this pull request Mar 2, 2021
* 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>
@lbdreyer lbdreyer deleted the eccodes_bug branch June 27, 2021 21: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.

4 participants