search

rom-py / rompy

Relocatable Ocean Modelling in PYthon (rompy) combines templated cookie-cutter model configuration with various xarray extensions to assist in the setup and evaluation of coastal ocean model
https://rom-py.github.io/rompy/
BSD 3-Clause "New" or "Revised" License
3 stars 9 forks source link

Flixible souce file #40

Closed rafa-guedes closed 1 year ago

rafa-guedes commented 1 year ago

@benjaminleighton I have implemented the changes to SourceFile we have discussed today. This object now has a reader optional field defaulting to "xarray.open_dataset" where you can define the reader function you want to use to open a file or file-like object into a Dataset. Some example using "xarray.open_mfdataset" is defined here.

The approach you have implemented in https://github.com/rom-py/rompy/pull/34 is also perfectly valid and would work fine. My personal preference though would be generalising SourceFile a bit as implemented here, and having less of these Source objects which should really only handle a few specific corner cases - for most cases we should be able to abstract other complexities using intake (I agree for example with the good suggestion from @pbranson in https://github.com/rom-py/rompy/issues/33).

The new reader field could also be passed as the function callable itself rather than a string, in case you are working programmatically from a Notebook for example.

benjaminleighton commented 1 year ago

Hi @rafa-guedes sorry for not getting to this last week. This looks good except that it might be a security risk depending on our threat model for serialized pydantic objects because allowing the user to execute many things by importing functions from standard python libraries see https://chat.openai.com/share/1d61a6e1-06ca-49dd-980f-b9d1d68dbaca

rafa-guedes commented 1 year ago

Hi @benjaminleighton, perhaps this may not be necessary anymore if we implement the dynamic intake catalog as proposed by Paul in https://github.com/rom-py/rompy/issues/43? If you still think we want the capability attempted to be generalise here but you are concerned this may represent a risk then we can implement it as you initially suggested with the SourceFiles class.

benjaminleighton commented 1 year ago

@rafa-guedes thanks, I'll close this as you suggest