Issue #1009: Retrieve single dataset - #1047
Conversation
| @@ -246,9 +246,9 @@ def mass_list_files_recursively(mass_path, simulation): | |||
| 'files': [] | |||
| } | |||
| datasets[dataset_id]['files'].append({ | |||
| 'filesize': elems[4], | |||
| 'filesize': elems[2], | |||
| 'filename': filename, | |||
| 'mass_path': elems[8] | |||
| 'mass_path': elems[6] | |||
There was a problem hiding this comment.
The data retrieval tool appeared to be broken when i tested it at the start of this issue. It was throwing this error:
File "CDDS/cdds/cdds/misc/retrieve_archived_data.py", line 333, in main_cdds_retrieve_archived_data
mass_file_list = mass_list_files_recursively(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "CDDS/cdds/cdds/common/mass.py", line 238, in mass_list_files_recursively
timestamp, filename) = elems[8].split('/')[-11:]
~~~~~^^^
IndexError: list index out of range
I can only assume that the 'moo', 'ls', '-Rl' command used here now has slightly different stdout formatting after the move to new MASS.
This change fixes the error so the tool works again.
|
Undertook a rename of the cdds specific tool in this commit from |
Edit: this comment is stale. See #1047 (comment)In this commit i created a replacement (called I considered modifying I've dropped this new function as a replacement for |
ae7a0a3 to
5c77cde
Compare
| "dataset_id", | ||
| help="Full CMIP6 dataset_id, e.g. CMIP6.CMIP.MOHC.UKESM1-0-LL.piControl.r1i1p1f2.Amon.tas.gn", | ||
| ) | ||
| if len(sys.argv) > 1 and sys.argv[1] == "get": |
There was a problem hiding this comment.
This guards against IndexError when no command is supplied - provides helpful output instead:
$ cdds/bin/crepp_retrieve_archived_dataset get
usage: crepp_retrieve_archived_dataset [-h] [--create-directories-false] [--mass-root MASS_ROOT]
[--dry-run] [--chunk-size CHUNK_SIZE]
{get,ls} dataset_id destination
crepp_retrieve_archived_dataset: error: the following arguments are required: dataset_id, destination
Arguably a bit brittle but the alternative was to use subparsers, which don't appear to be used elswhere in cdds - and they look a bit odd to me at first glance. So leaving this for ease of review unless that change is requested.
| """The :mod:`mass` module interact with the MASS archiving system.""" | ||
| import logging | ||
| import subprocess | ||
| import re |
There was a problem hiding this comment.
Didn't appear to be used
Pylance: "re" is not accessed
| def __init__(self, mass_failure, command): | ||
| super(MassError, self).__init__(mass_failure.get_message(command)) | ||
| self.msg = mass_failure.get_message(command) | ||
| self.mass_failure = mass_failure |
There was a problem hiding this comment.
Added this so callers can inspect which MassFailure caused an error, not just its rendered message.
| TMPDIR: str = tmpdir | ||
|
|
||
|
|
||
| def list_mass_files_with_checksums(mass_path: str) -> List[Dict[str, Any]]: |
There was a problem hiding this comment.
This function was inspired by mass_list_files_recursively in https://github.com/MetOffice/CDDS/blob/main/cdds/cdds/common/mass.py#L202
I was considering modifying that pre-existing one, but i think that would've had knock on effects if i messed around with it's interface.
Instead i decided to keep this logic in the module where it's used as i'm guessing the checksum functionality is something that will be exclusive to CREPP.
| # Please see LICENSE.md for license details. | ||
| import sys | ||
| from cdds.misc.retrieve_mass_data import main_cdds_retrieve_data | ||
| from cdds.misc.retrieve_archived_variables import main_cdds_retrieve_archived_variables |
There was a problem hiding this comment.
I undertook this rename of the previous tool to make the difference clearer to users that one is more for retrieving specific variables, whilst the other is for retrieving an entire dataset (also that the tools are specific to archived data as this has caused confusion for a user before).
So at command line they are now:
cdds_retrieve_archived_variables
cdds_retrieve_archived_dataset
| root = ET.fromstring(stdout_str) | ||
| for item in root.findall('node'): | ||
| # Skip directories and other non-file entries | ||
| if item.get('kind') != 'F': | ||
| continue | ||
| # Three following asserts largely exist to satisfy type checker as MASS should always provide them. | ||
| mass_file_path = item.get('url') | ||
| assert mass_file_path is not None | ||
| size_elem = item.find('size') | ||
| assert size_elem is not None | ||
| filesize = size_elem.text | ||
| checksum_elem = item.find('checksum/value') | ||
| assert checksum_elem is not None | ||
| checksum_value = checksum_elem.text | ||
| checksum = f"md5:{checksum_value}" |
There was a problem hiding this comment.
I went for element tree for parsing the XML. I did see sax used for a similar purpose in other areas of CDDS. From what i could see the only advantage sax has is when dealing with much larger amounts of XML than needed for this issue. Personally i find elementtree more readable, but don't mind refactoring this to use sax if you'd prefer.
More info on elementtree here: https://docs.python.org/3/library/xml.etree.elementtree.html
| # 1st: moo ls -Rlxm, 2nd: moo get -I -n (ls command is used to build paths the get uses for retrieval/dry run). | ||
|
|
||
| @patch(f"{_MODULE}.query_files_by_version") | ||
| def test_invalid_source_filepath_missing_status_returns_3(self, mock_query_files_by_version, tmp_path, caplog): |
There was a problem hiding this comment.
You'll find use of caplog and capsys in this test file. I can't see them used previously in CDDS. They're convenient built in pytest fixtures for capturing logger output or normal printed output.
More info:
https://docs.pytest.org/en/6.2.x/logging.html#caplog-fixture
https://docs.pytest.org/en/6.2.x/capture.html
| def test_success_returns_0_and_prints_json(self, _mock_run_mass_command, capsys): | ||
| result = run_ls_action(_CMIP6_FULL_DATASET_ID, _MASS_ROOT) | ||
| assert result == 0 | ||
| captured = capsys.readouterr() |
There was a problem hiding this comment.
Note: I like capsys, but i'm not a fan of their naming of this method.
At first i thought it meant it only read error output. It's actually supposed to mean it reads stdout AND stderr.
|
Both these tools have now been tested on MO and JASMIN and both seem to work for me: |
…rieve_dataset_for_CREPP
Matthew Mizielinski (matthew-mizielinski)
modified the milestones:
CDDS v4.0.2,
CDDS v4.1.0
2 participants
Closes #1009
Edit: Ended up opting for making a totally different script for this tool rather than adding the functionality to the pre-existing retrieve data tool as was originally planned. This was due to the CLI interfaces for the two intended uses becoming quite complicated and the code got quite convoluted when i attempted it. Also considering this tool is only intended for external CREPP users, i decided that was another reason to keep the code separate.