MHKiT-Software / MHKiT-Python

MHKiT-Python provides the marine renewable energy (MRE) community tools for data processing, visualization, quality control, resource assessment, and device performance.
https://mhkit-software.github.io/MHKiT/
BSD 3-Clause "New" or "Revised" License
50 stars 45 forks source link

Speed up Wind Toolkit tests #338

Closed akeeste closed 1 month ago

akeeste commented 3 months ago

This PR addresses #315 by:

akeeste commented 2 months ago

These changes were faster locally when data was pulled through the metocean example, but are still slower on actions (4h 2m) than the last wind hindcast cache test on master (2h 13m). I'll keep investigating but I have a hard time seeing a good path forward here

akeeste commented 2 months ago

Upon rerunning, the most recent wind hindcast cache test took 1h 43m, a substantial improvement if it is consistent. The rest of the hindcast tests only failed on the rerun because I did not rerun the wave hindcast upon which they depend. They passed on prior workflows

ssolson commented 2 months ago

That is a substantial improvement. Lets see if it is repeatable by merging the current development branch and pushing up to this feature branch. I am also interested to check that my hindcast logic for the notebook tests is working correctly.

Do you have any ideas for greater time improvements?

akeeste commented 2 months ago

@ssolson Will do on pulling the develop branch updates in.

I'm very doubtful whether or not this time improvement will be consistent. If not, I don't think there's a good way for us to actually pull data from the NREL server each time the test runs. One way we could eliminate the problem entirely is by committing the cached data and using it when testing these functions.

This would be less robust, but perhaps not that risky in the end. The biggest risk that I see is not consistently testing our dependency on NREL rex.

ssolson commented 2 months ago

Looks like the hindcast notebook logic worked and ran into an issue in the metocean example.

akeeste commented 2 months ago

I'll double check that example. I merged some changes and may have missed something.

akeeste commented 2 months ago

This would be less robust, but perhaps not that risky in the end. The biggest risk that I see is not consistently testing our dependency on NREL rex.

Apparently I tempted fate... I don't see anything obviously wrong with the notebook on our end.

Notes so far on the latest error:

@ssolson any ideas on what is going on with the metocean example?

ssolson commented 2 months ago

The notebook example data should be changed to match the wind station used in testing. The idea in the way the tests are designed is that we only hit the API once with the initial cache. Then reuse the data in all the tests.

Is it possible to switch the metocean example to use cached data (the same data used in the metocean tests)?

akeeste commented 2 months ago

Got it, thanks @ssolson. I was on the same page with the caching goal, but didn't realize that our testing workflow would actually prevent new data from being downloaded and cached outside of the prepare-wind-hindcast-cache job.

Yes I can add a test to test_wind_toolkit or change the data called in the metocean_example

ssolson commented 2 months ago

Got it, thanks @ssolson. I was on the same page with the caching goal, but didn't realize that our testing workflow would actually prevent new data from being downloaded and cached outside of the prepare-wind-hindcast-cache job.

Yes I can add a test to test_wind_toolkit or change the data called in the metocean_example

I was speaking to the tests efficiency, especially since we are already having trouble with the API we should avoid new calls if possible. Nothing in the tests is preventing making new API requests.

ssolson commented 2 months ago

It looks like the time was repeated at an hour and forty minutes 🥳. A huge improvement. image

ssolson commented 2 months ago

Looking at the error this morning I attempted a fresh install and I cannot recreate it locally.

I do not see any recent updates in NREL-Rex, h5py, h5pyd, or hsds.

The error says that the endpoint is not set for retrieving regional lat lon data. This leads me to think the issue is with accessing the .hscfg from the notebook (as no dependencies have recent updates).

However, the environment setup is consistent will all other calls to the hindcast APIs. So without being able to recreate the issue I will see if changing the example data fixes the issue and address based on the outcome.

ssolson commented 2 months ago

Looking at the re-run test failures the issue appeared for the wave hindcast example as well.

I added a step which will copy the .hscfg to the examples folder for the notebook run to see if this fixes the issue.

ssolson commented 2 months ago

The wind notebook is failing due to hitting a timeout of 20 minutes when hitting the wind hindcast API made by the following call in the final cell of the notebook:

wtk_temp, wtk_metadata = wind_toolkit.request_wtk_point_data(
   '1-hour',
   [
    'temperature_2m',
   'temperature_20m',
   'temperature_40m',
   'temperature_80m',
   'temperature_120m',
   'temperature_160m'
  ] ,
  ((40.748, -124.527),),
  [2018],
)

This differs from the cached test test_multi_parm in only that the the cached test asks for less parameters as shown:

wtk_multiparm, meta = wind_toolkit.request_wtk_point_data(
   '1-hour',
   [
   'temperature_20m',
   'temperature_40m',
  ] ,
  ((40.748, -124.527),),
  [2018],
)

Removing datapoints from the example would make the example less useful so I am going to try adding additional temperatures to the test call and see how it impacts the test time.