NASA-IMPACT / pangeo-notebook-veda-image

Docker container based on pangeo-notebook used on VEDA JupyterHub
https://gallery.ecr.aws/nasa-veda/pangeo-notebook-veda-image
MIT License
0 stars 2 forks source link

Update test_notebook.ipynb to include tests #4

Closed jsignell closed 2 months ago

jsignell commented 2 months ago

Does some mild testing of:

closes https://github.com/NASA-IMPACT/veda-jupyterhub/issues/27

batpad commented 2 months ago

@jsignell thank you so much!! 🚀 🚀

@sunu - would you be able to do a review here, and test that things look good and merge?

From a glance through the notebook, I think the only concern I have is with checking the output of catalog when opening with STAC. So these lines:

STAC_API_URL = "https://staging-stac.delta-backend.com/"
catalog = Client.open(STAC_API_URL)
catalog

I worry that our tests would break if anything about the catalog at https://staging-stac.delta-backend.com/ changes? Could we maybe only test the output of something more specific / or a query for a time-range that is unlikely to change?

Overall, this looks excellent and just what we needed to get going :-) Thanks again @jsignell !

sunu commented 2 months ago

I configured the GitHub Actions workflow to run builds and tests automatically upon pull request creation in https://github.com/NASA-IMPACT/pangeo-notebook-veda-image/pull/5. Let me close and reopen this PR to trigger the build.

jsignell commented 2 months ago

I think the only concern I have is with checking the output of catalog when opening with STAC.

Yes that is a very good point 🙈 maybe I should just do a repr of the item.

batpad commented 2 months ago

CI is behaving a bit weird, filed https://github.com/NASA-IMPACT/pangeo-notebook-veda-image/issues/6 - things work fine for me locally so not sure what's going on. We will debug that.

batpad commented 2 months ago

Hm sorry, as it stands, it seems like #6 is likely a blocker for doing more complex visualization tests. I think this would be good to figure out in the future, but I think for now we should whittle these tests down a bit and even if the code is complex, restrict the Outputs being tested to string representations.

For other things, we either would need to see if there's a way to get this working in pytest-notebook, or write specific pytest tests to test specific things.

I definitely think we need to continue working on improving the tests, including being able to run tests in cluster with s3 bucket permissions etc.

For now, @jsignell, do you know if you might have time to reduce the scope of the test notebook a bit to stick to string outputs for now? No worries if not, then will just make sure we prioritize this this week between me and @sunu - thanks again for your work here and really sorry we did not realize this limitation with pytest-notebook, but this is all very useful learning and should give us things to look into improving going forward.

jsignell commented 2 months ago

No worries! I can pare down the notebook later this week.

jsignell commented 2 months ago

Ok this version passes for me locally. I added some metadata to skip some of the output on import hvplot.xarray but we might want to skip more of the output from that one.

batpad commented 2 months ago

@jsignell you're the best! 🎉

This looks like it's passing CI as well! I'll just give things a quick look tomorrow and merge.

And then we can test upgrading to the latest pangeo base image and see how that goes.

Thanks again!

batpad commented 2 months ago

Am going ahead and merging this -

I imagine:

I think this is a great start and a significant improvement from what we had earlier (no tests).

@jsignell as we frame PI objectives for the next quarter, it would be great to discuss what we want to do here, and it would be amazing if we were able to continue getting some of your time to think about and improve tests here.

Ofc, feel free to continue making any changes / improvements here as you see fit, but I definitely think this is good to merge. Thanks @jsignell and @sunu for your work here!