Conversation
|
I think this is ready |
ghost
assigned
There was a problem hiding this comment.
I'm a bit concerned this isn't doing quite what it says on the tin.
I understand that we need cfchecker because test_pp_cf uses it.
But as far as I can see, it does not use its own copy of cf-standard-names.xml by default
-- instead, it will fetch it from STANDARDNAME = 'http://cf-pcmdi.llnl.gov/documents/cf-standard-names/standard-name-table/current/cf-standard-name-table.xml' (from cfchecker code)
In which case, it seems odd that this works at all, as it is ??
Meanwhile, what we used to do here was
- download a specific table and push it into lib/iris/etc
- create a custom cfchecker that refers specifically to that
I don't think we should be ditching that approach entirely here, so can we at least keep the old version commented out ?
I'm puzzled as to how the cfchecker test can be working now, if cfchecker is itself downloading from pcmdi.
As it happens, though, we do already have a copy of the version 23 'cf-standard-names.xml' burnt into the Iris repo at lib/etc
So I think it would be better to -
- comment out the xml download for now
- revert to the previous method of providing a 'custom' cfchecker, where the source is loaded from pypi (though you can up the version number if it looks good -- it seems surprising there is no 'latest stable' link, but I think you're right + there is not)
There was a problem hiding this comment.
After some offline chatting, I'm wondering why travis doesn't use the same version of the tables as the rest of us: https://github.com/SciTools/iris/tree/master/etc
There was a problem hiding this comment.
it seems odd that this works at all, as it is ??
From the Travis log:
/home/travis/build/SciTools/iris/lib/iris/tests/test_pp_cf.py:119: UserWarning: CF-netCDF "cfchecker" application not available. Skipping CF-netCDF compliance checking.
There was a problem hiding this comment.
In other words, the tests are looking for an executable named cfchecker, which is our own creation. It is a wrapper to cfchecks which sets the location of the lookup tables and UDUNITS. Having removed the creation of this script in this PR all the relevant tests are just skipping it.
There was a problem hiding this comment.
Thanks @pp-mo for offline chatting, and to @rhattersley.
If we can dig out a copy of area-type-table.xml, I suggest we add it to the Iris codebase,
and stop using this cfchecker script, using instead a call to cfchecks with pointers to Iris' copies of the files.
Advantages:
- no deps on breakable web resource
- anyone can run these tests, not just ukmo & travis
Any objections?
There was a problem hiding this comment.
stop using this cfchecker script, using instead a call to cfchecks with pointers to Iris' copies of the files
That's a separate issue from the one of the table downloads. I'd really like to keep it so.
Quite. Although we'd also need to include the area type table if we're to avoid being hamstrung by the flaky nature of the pcmdi.llnl.gov servers. |
See #968 for an attempt at this. |
Ah! You know what they say, "Great minds think alike". (Conincidentally, so do ours.) |
|
@cpelley I've just had a quick whizz through the It simply downloads the standard-name and area-type tables from:
using an So there's still no avoiding the |
No, see @rhattersley's comment above, it skipped the test |
Please read my earlier comment. The tests "passed" (or rather were skipped) because you deleted the |
|
#968 has worked ... I propose we merge that and leave this one closed. |
|
OK |
5 participants
Currently travis is failing as it is unable to return the tables from http://cf-pcmdi.llnl.gov
This PR updates the cfchecker, thereby using the cf standard names table distributed with the cfchecker itself, rather than relying on http://cf-pcmdi.llnl.gov