Get `nowcasting_dataset` running on OCF's on-premises server #152

JackKelly · 2021-09-22T15:27:27Z

Pull Request

Description

Modifies nowcasting_dataset so prepare_ml_data.py can be run on our local hardware, just by telling it to use the on_premises.yaml config.

Some other things this PR does:

tweaks some of the config arguments (e.g. consistently using *_zarr_path)
To minimise the amount of code we have to write to handle multiple compute environments, this PR makes a start on using Pathy and fsspec to provide a unified interface to GCP, AWS, and local (Use fsspec and/or pathy to provide a unified interface to GCP, AWS, and local filesystems #153)
I deleted example.yaml just to make our lives easier (so we don't have to continually update example.yaml). Either gcp.yaml or on_premises.yaml can be used as examples.
The config yaml files are (hopefully) a bit simpler and easier to understand now: Instead of my daft idea of being overly complicated and asking users to provide various base_paths, and then the code joins the base path to the other paths... the user now gives the full path for each field in the config. This ends up with slightly longer lines but hopefully it's more explicit and easier for users to understand what's going on; and also simplifies the code a little.

Fixes issue #121

TODO:

Check for other uses of some of the functions I've deleted / renamed

How Has This Been Tested?

It successfully runs on leonardo (there are occasional crashes, but those are unrelated to this PR, I think. See #158). And I've fixed all the unittests, so they all pass now :)

No
Yes

Checklist:

My code follows OCF's coding style guidelines
I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
I have checked my code and corrected any misspellings

…emises.py. No changes as of yet!

… local hardware. Does not work yet!

peterdudfield · 2021-09-22T21:15:16Z

Worth adding 'pathy' to help run tests

JackKelly · 2021-09-27T09:29:39Z

I'll work on fixing the failing tests and merging main into this branch...

JackKelly · 2021-09-27T13:35:03Z

conftest.py

@@ -64,7 +64,7 @@ def gsp_data_source():

 @pytest.fixture
 def configuration():
-    filename = os.path.join(os.path.dirname(nowcasting_dataset.__file__), "config", "example.yaml")


This PR deletes example.yaml (see the 'conversation' tab!)

JackKelly · 2021-09-27T13:45:21Z

environment.yml

@@ -14,17 +14,20 @@ dependencies:
  - zarr
  - xarray
  - ipykernel
-  - h5netcdf  # For opening NetCDF files from cloud buckets.


black made this change (and a bunch more like this). Turns out that black violates the pep8 recommendation to have two spaces before a comment, and I can't see how to force black to allow two spaces before comments, but ho-hum :)

JackKelly · 2021-09-27T13:52:23Z

nowcasting_dataset/cloud/utils.py

@@ -43,3 +44,38 @@ def gcp_to_aws(gcp_filename: str, gcs: gcsfs.GCSFileSystem, aws_filename: str, a
        upload_one_file(
            remote_filename=aws_filename, bucket=aws_bucket, local_filename=local_filename
        )
+
+
+def get_maximum_batch_id(path: str):


Moved from nowcasting_dataset/utils.py (and slightly modified)

JackKelly · 2021-09-27T13:53:27Z

nowcasting_dataset/config/gcp.yaml

@@ -28,7 +29,6 @@ process:
    - lcc
    - mcc
    - hcc
-  prcesion: 16


precision is left-over from when nowcasting_dataset was used during on-the-fly ML training. precision is only needed during ML training (not for creating batches), so removing this field

JackKelly · 2021-09-27T13:53:56Z

nowcasting_dataset/config/gcp.yaml

@@ -42,4 +42,3 @@ process:
    - VIS008
    - WV_062
    - WV_073
-  val_check_interval: 1000


val_check_interval is left-over from when nowcasting_dataset was used during on-the-fly ML training.

JackKelly · 2021-09-27T13:54:45Z

nowcasting_dataset/config/load.py

    Returns: pydantic class
    """

    # load the file to a dictionary
-    with open(filename, "r") as stream:
+    with fsspec.open(filename, mode="r") as stream:


fsspec.open() will open from local, GCP or AWS (it's magic!)

should we get ride of https://github.com/openclimatefix/nowcasting_dataset/blob/main/nowcasting_dataset/config/load.py#L28 then?

JackKelly · 2021-09-27T14:02:17Z

nowcasting_dataset/utils.py

@@ -133,38 +132,3 @@ def pad_data(
        data[variable] = pad_nans(data[variable], pad_width=((0, 0), pad_shape))  # (axis0, axis1)

    return data
-
-
-def get_maximum_batch_id_from_gcs(remote_path: str):


Moved to nowcasting_dataset/cloud/utils.py

peterdudfield · 2021-09-27T14:12:21Z

nowcasting_dataset/cloud/local.py

@@ -32,3 +32,9 @@ def delete_all_files_in_temp_path(path: Path):
    _LOG.info(f"Deleting {len(files)} files from {path}.")
    for f in files:
        os.remove(f)
+
+
+def check_path_exists(path: Union[str, Path]):


add doc string?

peterdudfield · 2021-09-27T14:16:03Z

nowcasting_dataset/config/save.py

        yaml.safe_dump(d, yaml_file, default_flow_style=False)
-
-
-def save_configuration_to_gcs(configuration: Configuration):


peterdudfield · 2021-09-27T14:17:11Z

nowcasting_dataset/dataset/batch.py


-
-def write_batch_locally(batch: List[Example], batch_i: int):
+def write_batch_locally(batch: List[Example], batch_i: int, path: Union[Path, Pathy]):


weoth adding a default of path = Path("~/temp/").expanduser()? or is this too hidden?

Ah, good question. Hmm... I think that, because ~/temp/ is set as the default in model.py, we perhaps don't want to set the default twice (i.e. in model.py and here?). But I don't have any strong feelings about this!

I have just added path to the docstring though (oops!)

peterdudfield · 2021-09-27T14:19:26Z

scripts/prepare_ml_data.py

+ENABLE_NEPTUNE_LOGGING = False
+
+# load configuration, this can be changed to a different filename as needed.
+# TODO: Pass this in as a command-line argument.


when this is done can we use Click. Is it worth making an issue for this? and then putting that issue number int eh code. Keeps the TODOs a bit tidier

I second Click!

Good idea! And thanks for letting me know about Click - I hadn't seen that before!

I've started an issue here.

I'll add a link in the code to the issue.

peterdudfield · 2021-09-27T14:20:41Z

scripts/prepare_ml_data.py

@@ -175,8 +211,7 @@ def main():
    iterate_over_dataloader_and_write_to_disk(datamodule.test_dataloader(), DST_TEST_PATH)
    _LOG.info("Done!")

-    # save configuration to gcs
-    save_configuration_to_cloud(configuration=config, cloud=CLOUD)
+    save_yaml_configuration(config)


 if __name__ == "__main__":


nice work making that much nice with the direct paths

Oh, I can't take any credit! fsspec does all the magic :) There's still more work to do to properly use fsspec throughout our code though (issue #153 tracks that)

jacobbieker

LGTM!

peterdudfield · 2021-09-27T14:21:34Z

tests/config/test.yaml

 output_data:
-  filepath: solar-pv-nowcasting-data/prepared_ML_training_data/v5/
+  filepath: NA


I like this, would it be even more verbose syaing filepath: not-setting-this-as-its-for-a-test

Good idea! Done!

peterdudfield

Looks really good - great to get it working on Leonardo - well done.

Ive added a few comments but they are all pretty minor, so choose to implement as you wish

peterdudfield · 2021-09-27T14:24:41Z

Actually something funny is happening with the CI?CD, so lets sort that before merging

peterdudfield · 2021-09-27T14:27:47Z

Actually something funny is happening with the CI?CD, so lets sort that before merging

Not sue why any of these new ones should cause an issue
s3fs
fsspec
pathy

…on leonardo!

…eter/on-premises # Conflicts: # requirements.txt

Origin/peter/on premises

JackKelly added 2 commits September 22, 2021 13:42

copying prepare_ml_training_data.py to prepare_ml_training_data_on_pr…

5f1ca67

…emises.py. No changes as of yet!

Tweaking config arguments, and making a start on preparing_ml_data on…

a4984c6

… local hardware. Does not work yet!

JackKelly added enhancement New feature or request data New data source or feature; or modification of existing data source infrastructure/on-premises hardware labels Sep 22, 2021

JackKelly added this to the WP1 essential tasks milestone Sep 22, 2021

JackKelly self-assigned this Sep 22, 2021

JackKelly linked an issue Sep 22, 2021 that may be closed by this pull request

Get nowcasting_dataset running on OCF's on-premises server #121

Closed

JackKelly mentioned this pull request Sep 22, 2021

Use fsspec and/or pathy to provide a unified interface to GCP, AWS, and local filesystems #153

Closed

nowcasting_dataset appears to be running on leoardo!

c2d04f3

JackKelly added 2 commits September 23, 2021 06:20

add fsspec and pathy to requirements and environment.yml

2945094

set num_workers=1 to see if that speeds up processing on leonardo

a70e737

peterdudfield removed this from the WP1 essential tasks milestone Sep 24, 2021

changing back to num_workers=4 #155

f983669

JackKelly added 5 commits September 27, 2021 09:36

merging main into this branch

ae2ae15

test_datamodule now passes

4322255

fix test_split_week_random

78887d8

OK, I think all tests should pass now :)

d7d7c0f

merging main into this branch

ff08180

JackKelly commented Sep 27, 2021

View reviewed changes

JackKelly marked this pull request as ready for review September 27, 2021 14:04

JackKelly requested review from peterdudfield and jacobbieker September 27, 2021 14:05

JackKelly mentioned this pull request Sep 27, 2021

In test_config.py, moto doesn't play nicely with s3fs #164

Closed

peterdudfield reviewed Sep 27, 2021

View reviewed changes

jacobbieker approved these changes Sep 27, 2021

View reviewed changes

peterdudfield reviewed Sep 27, 2021

View reviewed changes

peterdudfield approved these changes Sep 27, 2021

View reviewed changes

peterdudfield and others added 8 commits September 27, 2021 16:00

add s3fs to requirements

eb671ae

add fsspec to requirements

caa7459

pip install pathy

b300a13

updating on_premises.yaml and prepare_ml_data.py so it actually runs …

4db102f

…on leonardo!

Merge commit '4db102f8c917a69438ca755343683499ad6534a3' into origin/p…

e89f85e

…eter/on-premises # Conflicts: # requirements.txt

Merge pull request #167 from openclimatefix/origin/peter/on-premises

2bfee06

Origin/peter/on premises

Merge branch 'main' into jack/on-premises

2a195c9

implement code review suggestions. (Thanks Peter & Jacob)

32420dd

JackKelly merged commit d449fba into main Sep 28, 2021

JackKelly deleted the jack/on-premises branch September 28, 2021 15:51

JackKelly linked an issue Oct 1, 2021 that may be closed by this pull request

Get nowcasting_dataset running on OCF's on-prem hardware #111

Closed

JackKelly mentioned this pull request Oct 1, 2021

Get nowcasting_dataset running on OCF's on-prem hardware #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get `nowcasting_dataset` running on OCF's on-premises server #152

Get `nowcasting_dataset` running on OCF's on-premises server #152

JackKelly commented Sep 22, 2021 •

edited

Loading

peterdudfield commented Sep 22, 2021

JackKelly commented Sep 27, 2021

JackKelly Sep 27, 2021

JackKelly Sep 27, 2021

JackKelly Sep 27, 2021

JackKelly Sep 27, 2021

JackKelly Sep 27, 2021

JackKelly Sep 27, 2021

peterdudfield Sep 27, 2021

JackKelly Sep 27, 2021

peterdudfield Sep 27, 2021

peterdudfield Sep 27, 2021

peterdudfield Sep 27, 2021

peterdudfield Sep 27, 2021 •

edited

Loading

JackKelly Sep 28, 2021

peterdudfield Sep 27, 2021

jacobbieker Sep 27, 2021

JackKelly Sep 28, 2021

peterdudfield Sep 27, 2021

JackKelly Sep 28, 2021

jacobbieker left a comment

peterdudfield Sep 27, 2021

JackKelly Sep 28, 2021

peterdudfield left a comment

peterdudfield commented Sep 27, 2021

peterdudfield commented Sep 27, 2021

		yaml.safe_dump(d, yaml_file, default_flow_style=False)


		def save_configuration_to_gcs(configuration: Configuration):



		def write_batch_locally(batch: List[Example], batch_i: int):
		def write_batch_locally(batch: List[Example], batch_i: int, path: Union[Path, Pathy]):

Get nowcasting_dataset running on OCF's on-premises server #152

Get nowcasting_dataset running on OCF's on-premises server #152

Conversation

JackKelly commented Sep 22, 2021 • edited Loading

Pull Request

Description

How Has This Been Tested?

Checklist:

peterdudfield commented Sep 22, 2021

JackKelly commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdudfield Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobbieker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdudfield left a comment

Choose a reason for hiding this comment

peterdudfield commented Sep 27, 2021

peterdudfield commented Sep 27, 2021

Get `nowcasting_dataset` running on OCF's on-premises server #152

Get `nowcasting_dataset` running on OCF's on-premises server #152

JackKelly commented Sep 22, 2021 •

edited

Loading

peterdudfield Sep 27, 2021 •

edited

Loading